roachpb: use LocalUncertaintyLimit from err in readWithinUncertaintyIntervalRetryTimestamp #60021
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r8.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)
pkg/roachpb/data.go, line 1499 at r8 (raw file):
// global uncertainty limit. This, in turn, means that we can't rely on the // local uncertainty limit being a "clock timestamp". An alternative would // be to say that the local uncertainty limit is optional and only exists if
optionality does sound cleaner since it imposes a stronger invariant that it is never synthetic.
is it messy to plumb it as optional?
nvanbenschoten
force-pushed
the
nvanbenschoten/rwuiFollowerRead
branch
from
b6745db to
0632a64
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/roachpb/data.go, line 1499 at r8 (raw file):
Previously, sumeerbhola wrote…
optionality does sound cleaner since it imposes a stronger invariant that it is never synthetic.
is it messy to plumb it as optional?
Done over in #60021. I've now updated the comment to reflect this, along with the discussion we were having over there.
There was a problem hiding this comment.
Reviewed 1 of 9 files at r9.
Reviewable status:
pkg/roachpb/data.go, line 1499 at r8 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Done over in #60021. I've now updated the comment to reflect this, along with the discussion we were having over there.
Looks good.
It is a little bit nuanced that advancing past a synthetic timestamp corresponding to the uncertainty error is essential for liveness, so we do it, even though it may be in the future, but advancing past the global uncertainty limit is safe, not needed for liveness, and bad for performance, so we don't do it.
…ntervalRetryTimestamp Fixes cockroachdb#57685. This commit updates readWithinUncertaintyIntervalRetryTimestamp to use the the LocalUncertaintyLimit field from ReadWithinUncertaintyIntervalError in place of ObservedTimestamps directly in readWithinUncertaintyIntervalRetryTimestamp. This addresses a bug where ReadWithinUncertaintyIntervalErrors thrown by follower replicas during follower reads would use meaningless observed timestamps. Release note: None
nvanbenschoten
force-pushed
the
nvanbenschoten/rwuiFollowerRead
branch
from
0632a64 to
e1ae566
Compare
|
bors r+ |
|
Build failed (retrying...): |
|
Build succeeded: |
Fixes #57685.
This commit updates
readWithinUncertaintyIntervalRetryTimestampto use the theLocalUncertaintyLimitfield fromReadWithinUncertaintyIntervalErrorin place ofObservedTimestamps. This addresses a bug whereReadWithinUncertaintyIntervalErrorsthrown by follower replicas during follower reads would use meaningless observed timestamps.