jobs: break out progress batcher/rate limiter#36042
Merged
craig[bot] merged 2 commits intocockroachdb:masterfrom Mar 26, 2019
Merged
jobs: break out progress batcher/rate limiter#36042craig[bot] merged 2 commits intocockroachdb:masterfrom
craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
Member
|
This change is |
dt
commented
Mar 21, 2019
| // progressFractionThreshold. | ||
| var ( | ||
| progressTimeThreshold = 30 * time.Second | ||
| progressTimeThreshold = 15 * time.Second |
Contributor
Author
There was a problem hiding this comment.
this used to be checked via an OR with meaningful delta (5%) -- it is now checked via an AND, so that even if we make very fast progress, we don't spam the job with rapid-file progress updates.
Thus, it is now a lower-bound, not an upper-bound, which is why I changed it to 15s.
This extracts the logic for only sending a progress update when progress has meaningfully advanced and limiting their frequency from the existing logger, allowing it to be potentially reused elsewhere. Release note: none.
This was referenced Mar 22, 2019
pkg/jobs/progress.go
Outdated
| p.Unlock() | ||
|
|
||
| if shouldReport { | ||
| return p.Report(ctx, p.completed) |
Contributor
There was a problem hiding this comment.
The read on p.completed here needs to be within the mutex to prevent a race condition. Probably like completed := p.completed and then p.Report(ctx, completed) so Report doesn't lock the mutex.
Release note: none.
madelynnblue
approved these changes
Mar 26, 2019
Contributor
Author
|
bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Mar 26, 2019
36042: jobs: break out progress batcher/rate limiter r=dt a=dt This extracts the logic for only sending a progress update when progress has meaningfully advanced and limiting their frequency from the existing logger, allowing it to be potentially reused elsewhere. Release note: none. Co-authored-by: David Taylor <tinystatemachine@gmail.com>
Contributor
Build succeeded |
craig bot
pushed a commit
that referenced
this pull request
Mar 27, 2019
36106: importccl: parallelize CSV-skipping workload IMPORT r=dt a=dt (only last commit, first two commits are #36042) Reopening #36060 after accidentally deleting branch and then github said it cannot re-open :/ This spins up multiple workers, each importing every i'th batch, to do workload IMPORT. As noted inline, this execution order, as opposed to assinging large spans of batches to each worker, should mean that adjacent batches are processed at roughly the same time and thus end up in the same sort-batch for SST creation, preserving the non-overlapping SSTs when the workload's batches are ordered and non-overlapping. Release note: none. Co-authored-by: David Taylor <tinystatemachine@gmail.com>
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
3 participants
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.
This extracts the logic for only sending a progress update when progress has meaningfully advanced and limiting their frequency
from the existing logger, allowing it to be potentially reused elsewhere.
Release note: none.