kv: subject query intents to circuit breaker grace period #122255
Conversation
|
This change is |
There was a problem hiding this comment.
That was quick! A PR in these parts of the code can't be reviewed without minor refactor suggestions, so I left one for you in-line.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)
pkg/kv/kvclient/kvcoord/dist_sender_circuit_breaker.go line 128 at r1 (raw file):
func cbRequestKindFromBatch(ba *kvpb.BatchRequest, withCommit bool) cbRequestKind { if ba.IsWrite() || withCommit { return cbWriteRequest
It's unfortunate all of the naming here is coupled to read requests and write requests. How do you feel about switching this to something like:
type cbRequestCancellationPolicy int
const (
cbCancelImmediately cbRequestCancellationPolicy = iota
cbCancelAfterGracePeriod
cbNumCancellationPolicies // must be last in list
)Then we'd be able to add a comment explaining why ba.IsWrite() and withCommit are cancelled after a grace period (without making assumptions about what the caller is using the returned cbRequestKind field for).
nvanbenschoten
added
the
backport-24.1.x
Fixes cockroachdb#121997. This commit subjects the "pre-commit query intent" sub-batch of a parallel commit to the same circuit breaker grace period as writes (introduced in 9eed3b1). This prevents them from being eagerly cancelled on tripped DistSender circuit breakers, leading to ambiguous errors. Release note: None
nvanbenschoten
force-pushed
the
nvanbenschoten/withCommitGrace
branch
from
51d016a
to
f7178a4
Compare
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
pkg/kv/kvclient/kvcoord/dist_sender_circuit_breaker.go line 128 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
It's unfortunate all of the naming here is coupled to read requests and write requests. How do you feel about switching this to something like:
type cbRequestCancellationPolicy int const ( cbCancelImmediately cbRequestCancellationPolicy = iota cbCancelAfterGracePeriod cbNumCancellationPolicies // must be last in list )Then we'd be able to add a comment explaining why
ba.IsWrite()andwithCommitare cancelled after a grace period (without making assumptions about what the caller is using the returnedcbRequestKindfield for).
Good idea. Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:
|
This PR was included in a batch that was canceled, it will be automatically retried |
Fixes #121997.
This commit subjects the "pre-commit query intent" sub-batch of a parallel commit to the same circuit breaker grace period as writes (introduced in 9eed3b1). This prevents them from being eagerly cancelled on tripped DistSender circuit breakers, leading to ambiguous errors.
Release note: None