streamingccl: use correct bounds for range key SSTs #112903
Conversation
|
This change is |
| // Special case: The split key less than the start key. | ||
| if splitKey.Compare(start) < 0 { | ||
| return nil, &rangeKeySST{start: start, end: end, data: data}, nil | ||
| } | ||
|
|
||
| // Special case: The split key is greater or equal to the | ||
| // exclusive end key. | ||
| if end.Compare(splitKey) <= 0 { | ||
| return &rangeKeySST{start: start, end: end, data: data}, nil, nil | ||
| } |
There was a problem hiding this comment.
I could be convinced that these should just also return errors since they really should not happen.
There was a problem hiding this comment.
why don't you think this could happen? I think the RangeKeyMismatchError docstring suggests that any part of the rangekey sst we want to ingest could be in a different range-- I think that leaves open the possibility that the whole rangekey sst is in a different range.
There was a problem hiding this comment.
I think I was just not thinking this through when I wrote this. I think what I had in my mind is that if the whole SST was in a different range, then why did we ever send it to the wrong range. But, perhaps our entire SST is on one side of a split we don't know about yet. So, I'll leave these cases and remove the errant comment.
Thanks!
| // Special case: The split key less than the start key. | ||
| if splitKey.Compare(start) < 0 { | ||
| return nil, &rangeKeySST{start: start, end: end, data: data}, nil | ||
| } | ||
|
|
||
| // Special case: The split key is greater or equal to the | ||
| // exclusive end key. | ||
| if end.Compare(splitKey) <= 0 { | ||
| return &rangeKeySST{start: start, end: end, data: data}, nil, nil | ||
| } |
There was a problem hiding this comment.
why don't you think this could happen? I think the RangeKeyMismatchError docstring suggests that any part of the rangekey sst we want to ingest could be in a different range-- I think that leaves open the possibility that the whole rangekey sst is in a different range.
| @@ -956,7 +958,18 @@ func (r *rangeKeyBatcher) flush(ctx context.Context, toFlush mvccRangeKeyValues) | |||
| if err != nil { | |||
| return err | |||
| } | |||
| work = append([]*rangeKeySST{left, right}, work...) | |||
|
|
|||
| // The second two cases really should not be possible unless we've | |||
There was a problem hiding this comment.
again, curious why you think the second and third cases aren't really possible.
Previously, we were erroneously calling Next() on the end key of our SST even though the end key already represented an exclusive bounds. This Next() meant that the bounds of our SSTs were slightly incorrect. In most cases, this was harmless, but in the case of a range boundary that aligned directly with the _actual_ bounds of the SST, it could result in us attempting to split an SST into 2 with the RHS ending up empty. Here, we fix that and also make our SST splitting a bit more robust to bad input. It now errors if the LHS or RHS ends up empty despite a split key inside the given bounds. It also handles split keys outside of the bounds. Neither of these things should happen unless our contract with other parts of the system changes. Fixes cockroachdb#112846 Release note (bug fix): Fix a bug that could prevent phyiscal replication from advancing in the face of some range deletion operations.
stevendanna
added
backport-23.2.x
|
bors r=msbutler |
|
Build succeeded: |
Previously, we were erroneously calling Next() on the end key of our SST even though the end key already represented an exclusive bounds.
This Next() meant that the bounds of our SSTs were slightly incorrect. In most cases, this was harmless, but in the case of a range boundary that aligned directly with the actual bounds of the SST, it could result in us attempting to split an SST into 2 with the RHS ending up empty.
Here, we fix that and also make our SST splitting a bit more robust to bad input. It now errors if the LHS or RHS ends up empty despite a split key inside the given bounds. It also handles split keys outside of the bounds. Neither of these things should happen unless our contract with other parts of the system changes.
Fixes #112846
Release note (bug fix): Fix a bug that could prevent phyiscal replication from advancing in the face of some range deletion operations.