kv: mark AddSSTable requests "unsplittable" #24299

dt · 2018-03-28T19:27:37Z

Usually we split span requests automatically in DistSender however
AddSSTable requests are special: their large SST file is an opaque
payload that contains keys that could span the post-split requests.

We currently assert during ingestion that the span of an SST's contents
matches the request's span, as the file is ingested whole, without
modification, meaning that currently a split-and-truncated request
fails.

However we if a request will fail, we can reject it as soon as we'd try
to split it, before sending these large, expensive files around. More
importantly, if communicate back to the caller the actual range bounds,
it can try again, generating re-chunked SSTs with bounds that should be
OK.

Note: we could try to make the recipient able to correctly ingest only
the subset of the unsplit SST by iterating over it and generating a new
file containing just those keys in the request span. However this is
significant complexity to add to a raft command evaluation, and changes
the performance characteristics of AddSSTable that make it appealing
in the first place.

Release note: none.

cockroach-teamcity · 2018-03-28T19:27:42Z

This change is

dt · 2018-03-28T19:33:36Z

This is part of a collection of changes that teaches evalImport to chunk its SSTs so they don't cross range boundaries (including retrying chunks with updated splits when it gets this error back), but I wanted to pull the dist_sender change out for review on its own since I haven't really looked at distsender in a very long time so i'm probably making some silly mistakes.

As part of that larger CL, I have tests that cover this in 334a8fd#diff-1603e8373747484ccf4a1f6d121e8573R290, but would also be happy to have suggestion on if/how to test it in isolation too.

bdarnell · 2018-03-29T03:56:45Z

Several typos in commit message: reuquest, recipieant

Review status: 0 of 3 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

dt · 2018-03-29T14:40:23Z

pkg/kv/dist_sender.go

@@ -1125,6 +1130,9 @@ func (ds *DistSender) sendPartialBatch(
 			if err := evictToken.EvictAndReplace(ctx, replacements...); err != nil {
 				return response{pErr: roachpb.NewError(err)}
 			}
+			if ba.IsUnsplittable() {
+				return response{pErr: pErr}


huh, I think I messed this one up and it actually needs to move into divideAndSendBatchToRanges, right after the check if the batch fits within a (new) single range.

dt · 2018-03-29T15:19:51Z

Reworded the commit msg and moved one of the checks from just above the divideAndSendBatchToRanges in sendPartialBatch to inside divideAndSendBatchToRanges, after the check for a (new) single range.

tbg · 2018-03-29T18:13:41Z

Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

pkg/kv/dist_sender.go, line 858 at r2 (raw file):

				return
			}
			if ba.IsUnsplittable() {

Isn't this check redundant? ri hasn't been moved since the previous check and the conditions look the same. In fact I think the 1PC check could move up too, but if you want to minimize code movement you're better of removing the other check above.

Comments from Reviewable

dt · 2018-03-29T19:14:37Z

Review status: 2 of 3 files reviewed at latest revision, 2 unresolved discussions.

pkg/kv/dist_sender.go, line 858 at r2 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Isn't this check redundant? ri hasn't been moved since the previous check and the conditions look the same. In fact I think the 1PC check could move up too, but if you want to minimize code movement you're better of removing the other check above.

yep, you're right, once I moved the other call into divideAndSend, this one became redundant. deleted.
Done.

Comments from Reviewable

bdarnell · 2018-03-29T20:48:42Z

Review status: 2 of 3 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Comments from Reviewable

tbg · 2018-03-29T21:28:43Z

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

Comments from Reviewable

tbg · 2018-04-26T10:39:33Z

@dt, you're holding off on merging this intentionally, right? Just saw this in my list of open PRs.

dt · 2018-04-26T14:15:58Z

@tschottdorf yeah, it doesn't have test coverage here. I'd originally expected to land it together with changes in the calling code that actually use the error and would thus exercise it in tests, but we've re-ordered our backlog a bit and I haven't actually finished up that part of the change yet. I guess I could come up with some more isolated test and land this in isolation instead?

tbg · 2018-04-26T14:31:50Z

Your call, I was just curious.

On Thu, Apr 26, 2018 at 10:16 AM David Taylor ***@***.***> wrote: @tschottdorf <https://github.com/tschottdorf> yeah, it doesn't have test coverage here. I'd originally expected to land it together with changed in the calling code that actually use the error and would thus exercise it in tests, but we're working on some other features before the AddSST-based index backfill that required that, so I haven't finished that part of the change yet. I guess I could come up with some more isolated test and land this in isolation instead? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#24299 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE135GTerI-8qV_tqwOO6tSYp0X-mhI-ks5tsdasgaJpZM4S_OOq> .

--

…

-- Tobias

Usually we split span requests automatically in DistSender however AddSSTable requests are special: their large SST file is an opaque payload that contains keys that could span the post-split requests. We currently assert during ingestion that the span of an SST's contents matches the request's span, as the file is ingested whole, without modification, meaning that currently a split-and-truncated request fails. However we if a request will fail, we can reject it as soon as we'd try to split it, before sending these large, expensive files around. More importantly, if communicate back to the caller the actual range bounds, it can try again, generating re-chunked SSTs with bounds that should be OK. Note: we could try to make the recipient able to correctly ingest only the subset of the unsplit SST by iterating over it and generating a new file containing just those keys in the request span. However this is significant complexity to add to a raft command evaluation, and changes the performance characteristics of `AddSSTable` that make it appealing in the first place. Release note: none.

26811: kv: update the TCS's txn on requests way out r=andreimatei a=andreimatei The TxnCoordSender maintains a copy of the transaction record, used for things like heartbeating and creating new transactions after a TransactionAbortedError. This copy is supposed to be kept in sync with the client.Txn's copy. Before this patch, the syncing was done by updating the TCS's txn when a response comes back from a request. This patch moves to updating the TCS's txn on a request's way out, in addition to continuing to update it when a request comes back. Besides being the sane thing to do™, this assures that, if the heartbeat loop triggers before the response to the BeginTransaction's batch comes back, the transaction already has the key set. Without this patch, if the heartbeat loop triggered before the BeginTxn response, it would heartbeat key /Min, which is non-sensical (and creating load on range 0 for TPCC loadtests). Release note: None 26881: importccl: restart IMPORT on worker node failure r=mjibson a=mjibson Attempt to detect a context canceled error in IMPORT which is caused by a node going away in the dist SQL run. Send a special error back to the job registry indicating a restart should happen instead of a failure. We are shipping this with a skipped test because it is flakey. We are ok doing that because it is still better than what we had before in many cases, just not all. We will work to improve the other things so that we can correctly detect when IMPORT can be restarted due to a node outage, which will allow us to unskip this test. Fixes #25866 Fixes #25480 Release note (bug fix): IMPORT now detects node failure and will restart instead of fail. 26968: settings: bump minimum supported version to v2.0 r=nvanbenschoten a=nvanbenschoten We're currently shipping v2.1 alphas, so enforce a minimum binary version of v2.0. This ensures that no one can upgrade directly from v1.1 to v2.1. Instead, they need to make a pit sop in v2.0. Release note: None 26984: storageccl: retry SST chunks with new splits on err r=dt a=dt Simpler alternative to #26930. Closes #26930. Previously an ImportRequest would fail to add SSTables that spanned the boundries of the target range(s). This reattempts the AddSSTable call with re-chunked SSTables that avoid spanning the bounds returned in range mismatch error. It does this by iterating the SSTable to build and add smaller sstables for either side of the split. This error currently happens rarely in practice -- we usually explicitly split ranges immediately before sending an Import with matching boundsto them. Usually the empty, just-split range has no reason to split again, so the Import usually succeeds. However in some cases, like resuming a prior RESTORE, we may be re-Importing into ranges that are *not* empty and could have split at points other than those picked by the RESTORE statement. Fixes #17819. Subsumes #24299. Closes #24299. Release note: none. Co-authored-by: Andrei Matei <[email protected]> Co-authored-by: Matt Jibson <[email protected]> Co-authored-by: Nathan VanBenschoten <[email protected]> Co-authored-by: David Taylor <[email protected]>

dt requested review from tbg and a team March 28, 2018 19:27

dt requested a review from bdarnell March 28, 2018 19:51

dt force-pushed the unsplittable branch from 3a24096 to b4ec53f Compare March 29, 2018 14:13

dt commented Mar 29, 2018

View reviewed changes

dt force-pushed the unsplittable branch from b4ec53f to 7434adb Compare March 29, 2018 15:17

dt force-pushed the unsplittable branch from 7434adb to 11b5fc8 Compare March 29, 2018 19:13

dt force-pushed the unsplittable branch from 11b5fc8 to f6ddfd5 Compare March 30, 2018 04:26

dt force-pushed the unsplittable branch from f6ddfd5 to 4f9f0fc Compare June 19, 2018 21:10

This was referenced Jun 22, 2018

storageccl: re-split Import SSTables on range mismatch errors #26930

Closed

storageccl: retry SST chunks with new splits on err #26984

Merged

craig bot closed this in #26984 Jun 26, 2018

dt deleted the unsplittable branch June 26, 2018 21:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kv: mark AddSSTable requests "unsplittable" #24299

kv: mark AddSSTable requests "unsplittable" #24299

dt commented Mar 28, 2018 •

edited

Loading

cockroach-teamcity commented Mar 28, 2018

dt commented Mar 28, 2018

bdarnell commented Mar 29, 2018

dt Mar 29, 2018

dt commented Mar 29, 2018

tbg commented Mar 29, 2018

dt commented Mar 29, 2018 •

edited

Loading

bdarnell commented Mar 29, 2018

tbg commented Mar 29, 2018

tbg commented Apr 26, 2018

dt commented Apr 26, 2018 •

edited

Loading

tbg commented Apr 26, 2018 via email

kv: mark AddSSTable requests "unsplittable" #24299

kv: mark AddSSTable requests "unsplittable" #24299

Conversation

dt commented Mar 28, 2018 • edited Loading

cockroach-teamcity commented Mar 28, 2018

dt commented Mar 28, 2018

bdarnell commented Mar 29, 2018

dt Mar 29, 2018

Choose a reason for hiding this comment

dt commented Mar 29, 2018

tbg commented Mar 29, 2018

dt commented Mar 29, 2018 • edited Loading

bdarnell commented Mar 29, 2018

tbg commented Mar 29, 2018

tbg commented Apr 26, 2018

dt commented Apr 26, 2018 • edited Loading

tbg commented Apr 26, 2018 via email

dt commented Mar 28, 2018 •

edited

Loading

dt commented Mar 29, 2018 •

edited

Loading

dt commented Apr 26, 2018 •

edited

Loading