Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kv: add MVCC local timestamp details to ReadWithinUncertaintyIntervalError #96003

Merged

Conversation

nvanbenschoten
Copy link
Member

This PR adds details about MVCC local timestamps to ReadWithinUncertaintyIntervalErrors. This provides more information about the cause of uncertainty errors.

The PR also includes a series of minor refactors to clean up this code.

Release note: None
Epic: None

…lueTimestamp

Simple proto field rename. No compatibility concerns.

Release note: None
Epic: None
…s ClockTimestamp

Type check to avoid unnecessary upcasting. Improves type safety.

Release note: None
Epic: None
@nvanbenschoten nvanbenschoten requested review from a team as code owners January 26, 2023 15:44
@nvanbenschoten nvanbenschoten requested review from a team January 26, 2023 15:44
@nvanbenschoten nvanbenschoten requested review from a team as code owners January 26, 2023 15:44
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Only skimmed the surrounding code changes, since they appeared to be mostly mechanical.

@@ -950,17 +951,17 @@ func (e *ReadWithinUncertaintyIntervalError) SafeFormat(s redact.SafePrinter, _

func (e *ReadWithinUncertaintyIntervalError) printError(p Printer) {
p.Printf("ReadWithinUncertaintyIntervalError: read at time %s encountered "+
"previous write with future timestamp %s within uncertainty interval `t <= "+
"(local=%v, global=%v)`; "+
"previous write with future timestamp %s (local=%s) within uncertainty interval `t <= "+
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to omit the (local=%s) when it's equal to valueTs? Seems like that would be the common case, in which case it can be a bit noisy. No strong opinion either way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! Done.

…Error

This commit adds details about MVCC local timestamps to ReadWithinUncertaintyIntervalErrors.
This provides more information about the cause of uncertainty errors.

Release note: None
Epic: None
@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/uncertaintyErr branch from 71da68e to 812c5b9 Compare January 26, 2023 16:38
@nvanbenschoten
Copy link
Member Author

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Jan 26, 2023

Build succeeded:

@craig craig bot merged commit ed9928f into cockroachdb:master Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants