Say when the upload queue is filling, not only after it has lost something - #13
Merged
Merged
Conversation
…thing
A put writes the local body synchronously and queues the upload, and a
submission that finds the queue full is dropped. That is the right trade
for a best-effort tier, but the only evidence of it was a counter that
moves after the entries are already gone, and the loss surfaces on
another machine much later as an ordinary miss.
Report the backlog while it is still whole:
- Cache.UploadQueue reports depth and capacity, carried on the daemon's
status report and exposed as plaid_cache_upload_queue_depth and
plaid_cache_upload_queue_capacity. status prints the same pair.
- A drop logs a warning naming how many went with it, at most once per
30 seconds. A saturated queue drops thousands a second, so a line
each would bury the log; the count since the last report travels in
the line instead.
- PLAID_GOCACHE_UPLOAD_QUEUE_DEPTH replaces the hard-coded per-worker
backlog, so trading memory for burst tolerance is not a code change.
- PLAID_GOCACHE_UPLOAD_BLOCK_TIMEOUT lets a put wait a bounded time for
room instead of dropping. Off by default: a put that waits is a build
that waits, which is what the drop exists to prevent. Shutdown does
not wait it out either — close releases a waiting submit rather than
holding exit for the rest of an operator's timeout.
Defaults are unchanged: the same 64 slots per worker, and no waiting.
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.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
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.
A put writes the local body synchronously and queues the upload to the shared
tier. A submission that finds the queue full is dropped: the body is already on
disk, the put has already returned, and nothing fails or waits. That is the
correct trade for a best-effort tier and this change does not reverse it.
What the trade costs, though, was invisible while it was being paid. The only
evidence was a counter that moves after the entries are gone, and the loss
surfaces somewhere else entirely — a reader on another machine takes an ordinary
miss and redoes the work, with nothing about that miss to connect it back to the
queue that dropped the entry. Nothing said saturation was coming, and nothing
said it had happened.
What this adds
A backlog gauge.
Cache.UploadQueuereports depth and capacity. It rideson the daemon's status report, so it reaches all three readers at once:
plaid-cache statusprints anupload qline,status -fromprints it for adaemon you have no shell on, and
/metricsexposesplaid_cache_upload_queue_depthandplaid_cache_upload_queue_capacity. Bothare gauges — a backlog rises and falls, and this is the number that climbs
before anything is lost, where
uploads_total{result="dropped"}only movesonce it already has. They are deliberately not part of
Metrics: those arecounters persisted across processes, and a queue level means nothing once the
process holding it is gone.
A rate-limited warning on drop. The first drop logs immediately and no more
than one line is written per 30 seconds. A saturated queue drops thousands of
uploads a second, so a line each would bury the log it is meant to make
legible; each line carries the count since the previous one instead, which is
what distinguishes one overflowing burst from an hour of sustained shedding.
A submission arriving after shutdown is counted but not logged — that is not
the queue overflowing, and saying so would send a reader looking for load that
was never there.
PLAID_GOCACHE_UPLOAD_QUEUE_DEPTH, replacing the hard-coded per-workerbacklog. The right value is a property of the machine rather than of the tool:
the queue holds a job per waiting entry, so raising it buys tolerance of a
longer burst with memory. The constant remains as the default, so nothing
changes for anyone who does not set it.
PLAID_GOCACHE_UPLOAD_BLOCK_TIMEOUT, an opt-in bounded wait for roominstead of a drop. Off by default and it should stay off for ordinary
builds — a put that waits is a build that waits, which is precisely what the
drop exists to prevent. It is there for runs where the upload is the point and
the local copy is not, such as a warmer filling a bucket for everyone else. The
wait is bounded so opting in cannot become an unbounded stall, and shutdown does
not wait it out either:
closereleases a waiting submit before it takes thelock, so a configured timeout cannot delay process exit.
Defaults
Unchanged. The same 64 queue slots per worker, the same immediate drop, the same
counters. Nothing here can stall a writer unless an operator asks for it by name.
One thing left alone
The warning travels on the daemon's existing cache-diagnostics path, which is
gated at
PLAID_GOCACHE_LOG=info— the defaulterrorlevel silences it alongwith every other line the cache logs (index errors, upload failures, compactions).
Raising that path's level so cache failures are visible at the default verbosity
is a defensible change, but it would alter what every deployment writes to its log
file for reasons that have nothing to do with this queue, so it belongs in its own
decision. The two gauges need nothing turned on, and the README says which is
which.
Testing
go build ./...,go vet ./...,go test -race ./..., andgolangci-lint run --max-issues-per-linter=0 --max-same-issues=0 ./...all pass.New tests pin: the gauge reading a partly-full queue with no drops recorded yet;
exactly one warning per interval across many drops, naming the count; a put that
waits for room and lands rather than dropping; and that same wait expiring rather
than stalling.