More aggressive refetching when full

[bgentry]

Building on #663 and the benchmarks in #652, this tunes the client to be more aggressive about fetching when it either just fetched a full batch of jobs, or when it skipped its previously triggered fetch because it was already full. Rather than immediately calling the fetch limiter as in the previous commit, note this situation with a boolean so that we can trigger the fetch limiter after a worker slot has opened up.

This should bring more consistent throughput to poll-only mode and in cases where there is a backlog of existing jobs but new ones aren't being actively inserted.

This will result in increased fetch load on many installations, with the benefit of increased throughput. As before, FetchCooldown still limits how frequently these fetches can occur on each client and can be increased to reduce the amount of fetch querying.

Finally, don't run the fetch query if there are no worker slots available. I'm surprised we were doing this in the first place!

[bgentry]

@gaffneyc while looking more closely at this, I decided to adjust the approach from #663 to hopefully make it a little more efficient. I realized that there's a single producer goroutine responsible for deciding to fetch new jobs and for removing completed ones from the list of running ones. I also realized that the benchmarks in #652 and the solution proposed in #653 are going to be highly dependent on whether or not any jobs complete before the next attempted fetch. If the jobs are synthetic and ~instantaneous, there will always be slots available on the next attempt, but if they're realistic, odds are the fetch limiter may fire again before we have any slots available—and then we're back to polling!

I believe the approach in this PR is an improvement because it relies on a single boolean in the producer to let us delay triggering the fetch limiter until after we've freed up at least one worker slot. This means that the next triggered fetch will always have room for at least one job, whereas in #663 that's not the case. The boolean is always read and written by a single goroutine can avoid adding mutex overhead. It gets set to true either if the fetch limiter fired but we were already full, or if we fetched a full batch of jobs on the last fetch attempt. In either case it's a good signal that it's probably time for us to try to fetch jobs again as soon as we have space for them.

If possible, I'd love for you to run your benchmarks again with this branch. My guess is that the benchmarked throughput improvements will be the same (no-op jobs are always very fast), but in real-world use cases this change should be an improvement over your original proposal.

[bgentry]

Running river bench --duration 30s with these config changes, before and after this PR:

FetchCooldown:     100 * time.Millisecond,
FetchPollInterval: time.Second,
PollOnly:          true,

Before

bench: total jobs worked [      60000 ], total jobs inserted [     490000 ], overall job/sec [     2057.0 ], running 29.169128334s

After

bench: total jobs worked [     590002 ], total jobs inserted [     665000 ], overall job/sec [    19738.5 ], running 29.890884375s

[gaffneyc]

It's not clear to me how this is triggered only when a full batch has been returned. Should it only be set to true when len(results.jobs) == limit?

[bgentry]

Ugh, no, I totally missed that clause 🤦‍♂️ Fix incoming.

[bgentry]

Should be fixed in #668 / v0.14.1. Thanks for catching this 🙏

[gaffneyc]

I trust your knowledge of how River works better than mine and I didn't look into all the details so I'm sure there is something I missed. It is going to be a few days at least until I can cycle back here and test it on our code or do benchmarks.