[persist] Try and issue just one CaS for a shard at a time #34875
+48
−4
Conversation
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
bkirwi
force-pushed
the
state-semaphore
branch
2 times, most recently
from
8cb44d5 to
d3c7897
Compare
This is a little too strong, I think, but we'll see if it causes problems in testing.
A risk of a semaphore is that a single hung request holding a token ruins things for everyone. In general we'd much rather send concurrent requests -- which are possible anyways in a multiprocess system -- instead of risking deadlock.
bkirwi
force-pushed
the
state-semaphore
branch
from
d3c7897 to
17e65c2
Compare
DAlperin
approved these changes
Feb 3, 2026
Member
DAlperin
left a comment
DAlperin
left a comment
There was a problem hiding this comment.
one readability nit but otherwise LGTM. I like your semi-semaphore-with-timeout setup :)
| shard_metrics: &ShardMetrics, | ||
| state_versions: &StateVersions, | ||
| ) -> ApplyCmdResult<K, V, T, D, R, E> { | ||
| let _permit_opt = state.lease_for_update().await; |
Member
There was a problem hiding this comment.
nit: a comment here explaining why we hold the permit here would be nice
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Originally, Persist clients shared nothing aside from the underlying connection pool... if I had 20 clients for a particular shard, I'd have 20 different copies of state in memory being kept in sync. This was great for isolation, but wasteful of memory. Nowadays we keep just one copy in memory, but all 20 clients may still race to CaS it at once; this wastes a bunch of requests against the backing store, since only one of those 20 CaS operations can succeed, and 19 of them will fail. In particularly gnarly cases, this can get us into a durably bad state: almost all CaSs are failing and getting retried, causing the overall CaS rate to shoot up, meaning that new CaSs are even more likely to time out...
An obvious workaround is to limit each process to one outstanding CaS at a time. This is a little risky, though -- if we have a semaphore of limit 1, even 1 hung connection can cause all other clients to hang. If semaphore permits could time out, that would be ideal... but that's not how Tokio semaphores work. This PR implements its own little thing to solve that, and puts a flag around it so it can be tuned or disabled.
Motivation
https://github.com/MaterializeInc/incidents-and-escalations/issues/324 most recently, but I'm certain it's come up before.