Skip to content

kv: use txn deadline as MaxTimestampBound for bound staleness reads#69478

Merged
craig[bot] merged 2 commits intocockroachdb:masterfrom
nvb:nvanbenschoten/boundedStalenessMax
Aug 28, 2021
Merged

kv: use txn deadline as MaxTimestampBound for bound staleness reads#69478
craig[bot] merged 2 commits intocockroachdb:masterfrom
nvb:nvanbenschoten/boundedStalenessMax

Conversation

@nvb
Copy link
Copy Markdown
Contributor

@nvb nvb commented Aug 27, 2021

Fixes #69343.

This commit updates Txn.NegotiateAndSend to respect any prior set transaction
deadline. It does so by applying the deadline to the bounded staleness header's
MaxTimestampBound. This is necessary to prevent the bounded staleness read
from choosing a dynamic timestamp that is incoherent with the schema for the
table being read.

Release justification: fix to new functionality

@nvb nvb requested review from a team and otan August 27, 2021 16:49
@nvb nvb requested a review from a team as a code owner August 27, 2021 16:49
@nvb nvb requested a review from a team August 27, 2021 16:49
@nvb nvb changed the title kv: use txn deadline as MaxTimestamp for bound staleness reads kv: use txn deadline as MaxTimestampBound for bound staleness reads Aug 27, 2021
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@nvb nvb force-pushed the nvanbenschoten/boundedStalenessMax branch from 66f6a03 to e6b86c6 Compare August 27, 2021 16:50
Copy link
Copy Markdown
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r1, 9 of 9 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @otan)

nvb added 2 commits August 28, 2021 12:55
This commit clarifies the boundaries of the dynamic timestamp range represented
in BoundedStalenessHeader. It denotes MinTimestampBound as inclusive and
switches MaxTimestampBound from inclusive to exclusive. This change allows
MaxTimestampBound to more closely parallel EndTxnRequest.Deadline, which is also
exclusive. This is important because the next commit will begin setting bounded
staleness reads' MaxTimestampBound to the transaction deadline, when set.
Fixes cockroachdb#69343.

This commit updates Txn.NegotiateAndSend to respect any prior set transaction
deadline. It does so by applying the deadline to the bounded staleness header's
`MaxTimestampBound`. This is necessary to prevent the bounded staleness read
from choosing a dynamic timestamp that is incoherent with the schema for the
table being read.

Release justification: fix to new functionality
@nvb nvb force-pushed the nvanbenschoten/boundedStalenessMax branch from e6b86c6 to a4ef9b8 Compare August 28, 2021 16:56
@nvb
Copy link
Copy Markdown
Contributor Author

nvb commented Aug 28, 2021

bors r+

@nvb
Copy link
Copy Markdown
Contributor Author

nvb commented Aug 28, 2021

TFTR!

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Aug 28, 2021

Build failed (retrying...):

craig bot pushed a commit that referenced this pull request Aug 28, 2021
69478: kv: use txn deadline as MaxTimestampBound for bound staleness reads r=nvanbenschoten a=nvanbenschoten

Fixes #69343.

This commit updates Txn.NegotiateAndSend to respect any prior set transaction
deadline. It does so by applying the deadline to the bounded staleness header's
`MaxTimestampBound`. This is necessary to prevent the bounded staleness read
from choosing a dynamic timestamp that is incoherent with the schema for the
table being read.

Release justification: fix to new functionality

69493: server: make first node and store ID easy to change r=knz a=tbg

This is to enable the (sorry, internal) [runbook] to visualize a copy of the
internal timeseries data of a cluster. While writing the runbook I found some
sharp nails that this PR addresses. Mainly, the running node (n1) on top of
which the timeseries data is imported used to clobber some data for n1. The
interim solution is to start that server with a very high NodeID/StoreID (via
a manual code change & build). This isn't the most user-friendly, but it's
enough to unblock us and besides, the code changes here also constitute a
desirable cleanup.

[runbook]: https://cockroachlabs.atlassian.net/wiki/spaces/TS/pages/2168520776/Exporting+Internal+Timeseries+beta


Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Tobias Grieger <tobias.b.grieger@gmail.com>
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Aug 28, 2021

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Aug 28, 2021

Build succeeded:

@craig craig bot merged commit 4d9a6a3 into cockroachdb:master Aug 28, 2021
@nvb nvb deleted the nvanbenschoten/boundedStalenessMax branch August 29, 2021 22:25
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.

kv: use deadline as MaxTimestamp for bound staleness reads

3 participants