kvserver: improve error message for rare lease errors #64080
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @tbg)
In some rare cases, the status of a lease in relation to a request/timestamp can't be determined. For the request's client this results in a NotLeaseholderError. This patch improves the message of that error. In particular, this test failure[1] seems to show that a node couldn't verify that an existing lease is expired because its liveness gossiped info was stale. This sounds interesting and if the test fails again this improved message should help. [1] cockroachdb#57932 (comment) Release note: None
andreimatei
force-pushed
the
small.lease-error-msg
branch
from
8c2afa2
to
1f31803
Compare
| return nil, false, roachpb.NewError( | ||
| newNotLeaseHolderError(roachpb.Lease{}, r.store.StoreID(), r.mu.state.Desc, | ||
| "lease state couldn't be determined")) | ||
| newNotLeaseHolderError(roachpb.Lease{}, r.store.StoreID(), r.mu.state.Desc, msg)) |
There was a problem hiding this comment.
nit: probably no need to improve this since @knz thought what you have was fine, but if instead of a string you attached an EncodedError, wouldn't you retain proper redactability across the RPC and wouldn't it also feel "right" to attach an error to a lease with state ERROR?
There was a problem hiding this comment.
I agree with Tobias - I approved this because I knew that Andrei's main thrust was to remove the redaction markers and the PR achieves that, but moving to a structured error would be a nice improvement on top of that.
There was a problem hiding this comment.
👍🏽 In the interest of keeping things snappy I would merge as-is and we'll keep this in mind for higher-value use cases
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/replica_range_lease.go, line 1111 at r1 (raw file):
Previously, tbg (Tobias Grieger) wrote…
👍🏽 In the interest of keeping things snappy I would merge as-is and we'll keep this in mind for higher-value use cases
Well I would like to learn what the right thing is; it's not like this PR is particularly urgent. But I'm not entirely sure what the suggestion is. You're saying turn NotLeaseholderError.CustomMsg into an EncodedError (say, a cause field)? And then what would I do such that pErr.String() reaches intopErr.EncodedErr.(*NotLeaseholderError).Cause? I would implement make NotLeaseholderError implement SafeFormatter and rely on
cockroach/pkg/roachpb/errors.go
Line 224 in 3f38eb2
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/replica_range_lease.go, line 1111 at r1 (raw file):
Previously, andreimatei (Andrei Matei) wrote…
Well I would like to learn what the right thing is; it's not like this PR is particularly urgent. But I'm not entirely sure what the suggestion is. You're saying turn
NotLeaseholderError.CustomMsginto anEncodedError(say, acausefield)? And then what would I do such thatpErr.String()reaches intopErr.EncodedErr.(*NotLeaseholderError).Cause? I would implement makeNotLeaseholderErrorimplementSafeFormatterand rely on?cockroach/pkg/roachpb/errors.go
Line 224 in 3f38eb2
yes you could do that, to follow the current pattern.
I wouldn't call it "the right thing" though (see separate discussion on slack)
There was a problem hiding this comment.
bors r+
Reviewable status:
pkg/kv/kvserver/replica_range_lease.go, line 1111 at r1 (raw file):
Previously, knz (kena) wrote…
yes you could do that, to follow the current pattern.
I wouldn't call it "the right thing" though (see separate discussion on slack)
yeah... My enthusiasm diminished a little bit; I'm merging as is.
|
Build succeeded: |
In some rare cases, the status of a lease in relation to a
request/timestamp can't be determined. For the request's client this
results in a NotLeaseholderError. This patch improves the message of
that error.
In particular, this test failure[1] seems to show that a node couldn't
verify that an existing lease is expired because its liveness gossiped
info was stale. This sounds interesting and if the test fails again this
improved message should help.
[1] #57932 (comment)
Release note: None