Accept notified jobs for workers waiting on an empty buffer - #450
Open
salimepoint wants to merge 1 commit into
Open
Accept notified jobs for workers waiting on an empty buffer#450salimepoint wants to merge 1 commit into
salimepoint wants to merge 1 commit into
Conversation
accept? decided how many notified jobs to take from the free buffer space alone, so a locker running with maximum_size: 0 accepted none of them. Its buffer is always empty, so there is never free space and never anything buffered to displace, and every job waited for the next poll instead - up to poll_interval late, or indefinitely with polling off. A buffer size of zero is a reasonable setting for a queue of long running jobs, where a locker that pre-locks a job the running job goes on to wait for will deadlock. Fall back to the workers currently blocked waiting for a job when the buffer has no space to offer. That is safe because push hands jobs straight to waiting workers, before it trims the buffer to maximum_size, so a job claimed by an idle worker never occupies buffer space and is never pre-locked. available_priorities already sizes the poller's fetch as the waiting workers plus the buffer space, so accept? was the one place left that ignored them. Falling back only when there is no space leaves accept? behaving exactly as it did before whenever the buffer has room to fill. With a full buffer it now also takes what the waiting workers will accept on top of what it can displace; push places those and returns the remainder for push_jobs to unlock, as it does for any other overage. The count is priority aware, since a worker only accepts jobs its threshold admits. metajobs is sorted and a threshold admits every job at least as important as the ones it rejects, so the eligible worker count only falls as the walk advances and the first job that cannot be placed for certain ends it. The walk is therefore conservative rather than exact: push offers each job to the most permissive waiting worker first, so counting every placement a perfect assignment could make would accept jobs push then has to displace.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
JobBuffer#accept?decided how many notified jobs to take from the free buffer space alone:start_index = _buffer_spaceSo a locker running with
maximum_size: 0accepted none of them. Its buffer is always empty, so there is never free space and never anything buffered to displace — every notified job was rejected and left to be found by the next poll instead, up to poll_interval late, or indefinitely with polling turned off.maximum_size: 0is a reasonable setting for a queue of long-running jobs, where a locker that pre-locks a job the running job goes on to wait for will deadlock. Those are exactly the deployments that then get no benefit from NOTIFY at all.The fix
Fall back to the workers currently blocked waiting for a job when the buffer has no space to offer.
That's safe because push hands jobs straight to waiting workers (via priority_queues … pq.populate) before it trims the buffer to maximum_size — so a job claimed by an idle worker never occupies buffer space and is never pre-locked. available_priorities already sizes the poller's fetch as the waiting workers plus the buffer space, so accept? was the one place left that ignored them.
Behaviour is unchanged whenever the buffer has room to fill, since the fallback only engages when there is no space at all. With a full buffer, accept? now also takes what the waiting workers will accept on top of what it can displace; push places those and returns the remainder for push_jobs to unlock, as it already does for any other overage.
Priority awareness
The count has to be priority-aware, since a worker only accepts jobs its threshold admits. metajobs is sorted, and a threshold admits every job at least as important as the ones it rejects, so the eligible worker count only falls as the walk advances — the first job that can't be placed for certain ends it.
The walk is deliberately conservative rather than exact: push offers each job to the most permissive waiting worker first, so counting every placement a perfect assignment could make would accept jobs that push then has to displace.
Tests
New coverage in job_buffer_spec for: no worker waiting (still rejects); accepting no more than the waiting-worker count; an accepted job reaching the worker without ever being buffered; spare buffer space still capping acceptance regardless of waiting workers; a full buffer taking the waiting worker's job on top of what it displaces; and a worker whose priority threshold does and doesn't admit the offered job.