decider: ignore EnsureSafeSplitKey on error #43078
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained
Fixes cockroachdb#42903. Load based splitting would previously not split when EnsureSafeSplitKey returns an error. However such an error only meant that the key in question wasn't even a SQL key, which is the case most of the time, at least in simple workloads such as kv (the SQL layer will send single-element scans, but without specifying the column family). No release note since not released. Release note: None
|
You're right, test added. TFTR! |
| if err != nil { | ||
| key = nil | ||
| // | ||
| // Note that we ignore EnsureSafeSplitKey when it returns an error since |
There was a problem hiding this comment.
I don't get it - the comment above says that we don't want to allow mid-row splits and that SQL even crashes.. If this changed we should update that comment
There was a problem hiding this comment.
Oh, or we know for sure that in the error case the key is "in-between" rows? Seems like we should fix EnsureSafeSplitKey to not return an error at all, or at least explain this in its comment
There was a problem hiding this comment.
SQL sort of forces us to avoid mid-column family splits and we can't do so because you can't tell the column family suffix if you don't know the SQL schema. Also SQL issues plenty of scans that don't have the column family in the start key, so that's what the split finder sees. So when it sees a key that doesn't look like a SQL key, it has to assume that it doesn't cut columns in half, or it couldn't split most of the time (as we saw in the test this commit fixes). This probably holds but if you issued unfortunate batch requests you can definitely trick it into still splitting between col families.
I'm pretty unhappy about the situation in here and it seems reasonable to make SQL resilient to col families being split between ranges, not because I love that, but because I don't see any other way 😞
There was a problem hiding this comment.
Bleah. I would put these details in an issue and mention the issue in the comment.
|
bors r=nvanbenschoten |
43078: decider: ignore EnsureSafeSplitKey on error r=nvanbenschoten a=tbg Fixes #42903. Load based splitting would previously not split when EnsureSafeSplitKey returns an error. However such an error only meant that the key in question wasn't even a SQL key, which is the case most of the time, at least in simple workloads such as kv (the SQL layer will send single-element scans, but without specifying the column family). No release note since not released. Release note: None Co-authored-by: Tobias Schottdorf <[email protected]>
Build succeeded |
Fixes #42903.
Load based splitting would previously not split when EnsureSafeSplitKey
returns an error. However such an error only meant that the key in
question wasn't even a SQL key, which is the case most of the time,
at least in simple workloads such as kv (the SQL layer will send
single-element scans, but without specifying the column family).
No release note since not released.
Release note: None