Conversation
…ny priority rather than all priorities
…g jobs to workers when a match is found
… skip the remaining waiting workers for a priority if that priority can't be satisfied
ZimbiX
force-pushed
the
make-poller-more-easily-satisfied
branch
from
1c4bc57 to
ef2a67d
Compare
| worker_job_counts[worker_priority] += 1 | ||
| end | ||
| worker_job_counts[worker_priority] == waiting_workers_count | ||
| end |
There was a problem hiding this comment.
Yeah, I see how this is more complicated than it first sounded, nice work.
I want to understand the relationship between the buffer size, the available_priorities hash and what is returned by the poller a bit better. I'm wondering whether a better way to determine whether a poll is satisfied is if we collect at least enough jobs to fill whatever buffer space is available (and maybe at least one priority?, but that feels like an anti-optimisation given the complexity of implementing it). My understanding at the moment is that the counts in job_buffer.available_priorities sum to the number of available workers plus the available space in the buffer. If we can fill the buffer then I think that represents a satisfactory poll, regardless of whether some workers didn't get a job.
There was a problem hiding this comment.
Just had another thought, maybe all that we care about is that the lowest priority worker queue is full?
The available space in the buffer is added to the lowest priority worker queue when constructing the available_priorities hash. So if the lowest priority worker queue is full then the buffer will be full and all available workers of the lowest priority will have a job to do. I think that is a pretty good metric to use for last_poll_satisfied.
There was a problem hiding this comment.
Just thinking this out:
Five available p1 workers, five available p50 workers, available buffer space of 10, the available priorities will be
{ p1: 5, p50: 15 }
| p1 jobs | p2 jobs | result | p50 satisfied? |
|---|---|---|---|
| 100 | 3 | { p1: 5, p50: 15 } |
yes |
| 3 | 100 | { p1: 3, p50: 15 } |
yes |
| 10 | 10 | { p1: 5, p50: 15 } |
yes |
| 9 | 10 | { p1: 5, p50: 14 } |
no |
I think I am ok with the last one not being satisfied, the main thing I want to fix here is not polling again immediately when there are still HEAPS of jobs to work.
There was a problem hiding this comment.
But, maybe that doesn't actually simplify anything because to determine whether the lowest priority has been satisfied we need to do exactly what you have done and allocate jobs to higher priorities.
Another thought: a more fuzzy way of doing this is to just compare the number of jobs to the lowest priority availability without being too exact about where the jobs will actually end up?
Just thinking this out:
Five available p1 workers, five available p50 workers, available buffer space of 10, the available priorities will be
{ p1: 5, p50: 15 }
| p1 jobs | p2 jobs | total job | total jobs > p50 availability? (i.e. last_poll_satisfied?) |
|---|---|---|---|
| 100 | 3 | 20 | yes |
| 3 | 100 | 18 | yes |
| 10 | 10 | 20 | yes |
| 9 | 10 | 19 | yes |
| 4 | 10 | 14 | no |
| 10 | 4 | 14 | no |
That looks pretty good to me. Will see if I can break it.
There was a problem hiding this comment.
Haven't tested it, think I still need to deal with the nil priority but something like this:
def poll_satisfied?(priorities, jobs)
lowest_priority = priorities.keys.min
jobs.count >= priorities[lowest_priority]
endThere was a problem hiding this comment.
If the number of low priority workers plus the buffer size is greater than the total number of non-lowest-priority workers (as is the case with the default settings priorities: 10,20,50,any,any,any and buffer: 8) then it's very difficult to think of a case where you get more than one potentially unnecessary poll occasionally.
There was a problem hiding this comment.
Just to clarify, #349 never intended to answer the question of whether any requested priority is full of jobs. It's a different question, which I think provides a close enough approximation.
There was a problem hiding this comment.
My main concerns with this algorithm are the ones you raised yourself: that it's complex, a bit impenetrable and that we're re-implementing worker allocation which introduces a coupling we'd probably rather not have. Happy to go with it though if you don't think #349 is appropriate.
There was a problem hiding this comment.
Ah, I see. I suppose for a rough algorithm, it is better that it err on the side of caution - sometimes polling again unnecessarily, rather than mistakenly sleeping.
Your code is way more readable than mine. And yeah, I can't see that there's much of a drawback either. So I'm leaning towards going with that. But I'd like to let it sit for a while to see how I feel about it later.
if there is a steady stream of jobs one could argue that polling continuously is actually the preferred behaviour, even if workers are not being fully utilised?
I would have thought the notify would handle most/all of that case; but I'm not totally across it.
There was a problem hiding this comment.
I'd like to let it sit for a while to see how I feel about it later.
Sure, sounds good.
I would have thought the notify would handle most/all of that case; but I'm not totally across it.
Oh yeah! I completely forgot about that, you’re probably right.
|
Ah crap, wrong button!! |
2 participants
Resolves #345.
Basically, this is all about polling again immediately if there are a decent number of jobs we need to get through. But 'decent number of jobs' needs a good definition.
Previously, all idle worker priorities needed to be fully utilised by the jobs from a poll in order to not sleep before the next poll. This change makes it so that sleeping is skipped if any waiting worker priority is fully satisfied by the jobs from the poll.
Determining whether a worker priority would be fully utilised turned out to be far less trivial than I first thought it'd be. You have to take into account that jobs with a higher priority (a smaller number) than a worker's priority also match it.
I'm sure there's a more functional programming way to do this, but I couldn't think of it!
This was kinda hard to explain, so wording suggestions more than welcome.
A simpler alternative would be to just sleep if there are no jobs returned from the poll. That'd incur an extra poll when only a small number of jobs come in, but might be worth the ability to avoid this PR's increase in code complexity? I've kinda had to reimplement allocating jobs to workers!
Manually tested: