sql: add RevertTable wrapper for RevertRangeRequest#39206
sql: add RevertTable wrapper for RevertRangeRequest#39206craig[bot] merged 3 commits intocockroachdb:masterfrom
Conversation
|
This change is |
nvb
left a comment
There was a problem hiding this comment.
Reviewed the Core portion of this.
Reviewed 1 of 1 files at r2, 4 of 4 files at r3.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @adityamaru27, @dt, @lucy-zhang, and @nvanbenschoten)
pkg/storage/batcheval/cmd_revert_range.go, line 101 at r3 (raw file):
log.VEventf(ctx, 2, "hit limit while clearing keys, resume span [%v, %v)", resume.Key, resume.EndKey) reply.ResumeSpan = resume reply.NumKeys = cArgs.MaxKeys
I think we should be returning this from MVCCClearTimeRange and setting it unconditionally.
There was a problem hiding this comment.
Reviewable status:
pkg/storage/batcheval/cmd_revert_range.go, line 101 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
I think we should be returning this from
MVCCClearTimeRangeand setting it unconditionally.
I initially didn't want to set it at all -- unlike, say, ScanRequest, I don't care about how many total keys I've touched since there isn't a result set of that size coming back to me. The MaxScanRequestKeys is being set to, and only to, limit the max batch sizes. That said, I found that distsender didn't behave well with multiple ResumeSpans, so I ended up setting it when, and only when, returning a resume span, to make DistSender break out of its send loop, and thus we want to set it to exactly the limit.
If we were actually always returning the number of keys cleared or the number of writes in the batch, then I could see having it come from MVCCClearTimeRange, but we don't want to set it to either of those, since the first doesn't make sense if we're able to clear thousands of keys with a single ClearRange, and both don't really make sense when they debit from an overall limit shared access requests. Since the the number being returned here is being picked solely for distsender's benefit, it felt odd to have that logic in MVCCClearTimeRange.
thoszhang
left a comment
There was a problem hiding this comment.
I reviewed the sql part.
Reviewed 3 of 3 files at r1, 1 of 1 files at r2.
Reviewable status:
nvb
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/storage/batcheval/cmd_revert_range.go, line 101 at r3 (raw file):
Previously, dt (David Taylor) wrote…
I initially didn't want to set it at all -- unlike, say, ScanRequest, I don't care about how many total keys I've touched since there isn't a result set of that size coming back to me. The MaxScanRequestKeys is being set to, and only to, limit the max batch sizes. That said, I found that distsender didn't behave well with multiple ResumeSpans, so I ended up setting it when, and only when, returning a resume span.
If we were actually always returning the number of keys cleared (certainly don't want to do that since I don't want to bail early if a single ClearRange could get them all), or the number of writes in the batch (Clears+ClearRanges), then I could see having it come from MVCCClearTimeRange, but given that the number returned here is being picked for distsender's benefit, it felt odd to have that logic in MVCCClearTimeRange.
Ok, this all sounds reasonable to me. Please document this here then.
dt
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/storage/batcheval/cmd_revert_range.go, line 101 at r3 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Ok, this all sounds reasonable to me. Please document this here then.
Done.
Release note: none.
This patch includes several changes required to make RevertRange requests get along with DistSender: 1) add RevertRange to the explicit check for maxkey-supported requests. 2) makes RevertRangeResponse combinable, however combining assumes that at-most-one reply returned a ResumeSpan, so it also 3) sets NumKeys to MaxKeys in RevertRange when returning a ResumeSpan, to stop distsender from sending any further requests that might also hit a resume and then trip up combining. Together these make it possible to send batches of RevertRange requests via DistSender and get back resume spans. Release note: none.
This adds a helper that can revert a collection of tables to a target timestamp using the RevertRange RPC. It checks that the tables do not overlap the key space of another table not being reverted (i.e. that all the reverting tables are interleaved, if at all, only with each other) and that the tables are offline. It then reverts the table spans using RevertRange, including handling the ResumeSpan pagination. Release note: none.
|
bors r+ |
|
looks like bors timed out bors r+ |
39206: sql: add RevertTable wrapper for RevertRangeRequest r=dt a=dt (first commit is #39251) kv,batcheval: fix RevertRange to work with DistSender and ResumeSpans This patch includes several changes required to make RevertRange requests get along with DistSender: 1) add RevertRange to the explicit check for maxkey-supported requests. 2) makes RevertRangeResponse combinable, however combining assumes that at-most-one reply returned a ResumeSpan, so it also 3) sets NumKeys to MaxKeys in RevertRange when returning a ResumeSpan, to stop distsender from sending any further requests that might also hit a resume and then trip up combining. Together these make it possible to send batches of RevertRange requests via DistSender and get back resume spans. Release note: none. sql: add RevertTables helper for reverting a Table via RevertRange This adds a helper that can revert a collection of tables to a target timestamp using the RevertRange RPC. It checks that the tables do not overlap the key space of another table not being reverted (i.e. that all the reverting tables are interleaved, if at all, only with each other) and that the tables are offline. It then reverts the table spans using RevertRange, including handling the ResumeSpan pagination. Release note: none. 39427: storage/engine: don't decrement IntentCount on system intent abort r=nvanbenschoten a=nvanbenschoten Fixes #39390. Fixes #39407. Fixes #39370. Fixes #39419. A bug introduced in #39251 was causing the IntentCount field in MVCCStats to get out of sync. I was expecting `TestMVCCStatsRandomized/sys/inline=false` to easily catch the bug, but it wasn't. It turns out that the test was updating the transaction's write timestamp but not its read timestamp, which was causing it to hit this error: https://github.com/cockroachdb/cockroach/blob/41cb28e85575292926790e79912c893d13e1054d/pkg/storage/engine/mvcc.go#L1349 This was preventing the randomized test from performing the series of steps we needed it to hit the bug. This commit fixes the bug, fixes the randomized test so that it would have caught it, and adds a targeted regression test for this case. Release note: None Co-authored-by: David Taylor <tinystatemachine@gmail.com> Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Build succeeded |
(first commit is #39251)
kv,batcheval: fix RevertRange to work with DistSender and ResumeSpans
sql: add RevertTables helper for reverting a Table via RevertRange