storageccl: retry SST chunks with new splits on err #26984
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
|
Review status: Comments from Reviewable |
complete! 0 of 0 LGTMs obtained (and 1 stale)
|
I should mention the simplicity here does come at a (small?) price: First there's the extra memory of keeping an extra SST in memory while we produce the split ones, rather than going back to the original source and reading from it directly. Additionally, this approach could result in more SSTs, since the approach in #26930 would start a new, full-sized chunk at the split, rather than just the remainder of the rejected chunk, which could be very small, e.g. if the split is very near the end of a chunk. The former I think is probably OK and as long we we aren't making new ranges based on the chunking here, I think the later is OK too -- a small chunk just means slightly more overhead but shouldn't change the result. |
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.
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 cockroachdb#17819. Release note: none.
maddyblue
approved these changes
Jun 26, 2018
|
bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Jun 26, 2018
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]>
Build succeeded |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.