kvnemesis: add support for AddSSTable
#89145
Conversation
|
This change is |
|
@tbg Would be great if you could add in support for ingestion of MVCC point/range tombstones once you wrap up the work on deletion validation. |
erikgrinaker
force-pushed
the
kvnemesis-addsstable
branch
2 times, most recently
from
18e5525 to
0d94aa1
Compare
erikgrinaker
force-pushed
the
kvnemesis-addsstable
branch
3 times, most recently
from
e64b6b5 to
a54731f
Compare
erikgrinaker
force-pushed
the
kvnemesis-addsstable
branch
from
a54731f to
c162334
Compare
|
@tbg This is ready for review now. With the attached bug fixes I had 20000 passes. |
erikgrinaker
force-pushed
the
kvnemesis-addsstable
branch
from
c162334 to
770c480
Compare
pkg/kv/kvnemesis/generator.go
Outdated
| rangeEnd = ranges[rangeIdx+1] | ||
| } | ||
| inRange := func(key string) bool { | ||
| return key >= rangeStart && (key < rangeEnd || rangeEnd == "") |
There was a problem hiding this comment.
All keys are encoded uints, check the fk and tk methods. You can do int arithmetic for everything and this is how we do it elsewhere, I would prefer we stick to one pattern because each way of interacting with their keys has its idiosyncrasies and diversity doesn't pay off.
pkg/kv/kvnemesis/validator.go
Outdated
| // error and try again later. | ||
| } else if resultIsErrorStr(t.Result, "write for key .* at timestamp .* too old; wrote at") { | ||
| // We may race with a concurrent write. Try again later. | ||
| } else if v.checkNonAmbError(op, t.Result) { |
There was a problem hiding this comment.
I definitely am not doing anyone a favor with this v.checkX methods, I needed to read comments two layers deep to remember how this works again. When looking at this line I was like "are you just ignoring errors here?!" but v.checkNonAmbError will emit a failure to the validator on non-exempt errors.
There was a problem hiding this comment.
Added a comment to clarify this.
| @@ -1807,6 +1827,16 @@ func TestValidate(t *testing.T) { | |||
| rd(k1, k2, t2, s2), | |||
| ), | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
Is it worth pairing this with at least one or two other interactions with other commands? I've seen the verifier code and the changes are fairly trivial so we probably don't need too much else.
There was a problem hiding this comment.
Yeah, may as well. Added some shadowing and scans.
pkg/kv/kvnemesis/watcher.go
Outdated
| case *roachpb.RangeFeedSSTable: | ||
| w.mu.Lock() | ||
| if len(e.Data) == 0 { | ||
| return errors.AssertionFailedf("no SST data found") |
There was a problem hiding this comment.
The locking here is broken, lots of error returns that never unlock.
What I don't understand is why the other code branches do this too, is this ok for some reason?
There was a problem hiding this comment.
Could you pull this out into w.handleAddSSTable? That will solve your locking problem and also declutter this method.
If you feel generous you could do the same for the rangefeed checkpoint case above.
There was a problem hiding this comment.
The test will fail and exit on any errors, so it doesn't really matter that we're holding the lock, and I figured since we already did this for other branches it was "fine". Anyway, will pull them out into separate methods.
pkg/kv/kvnemesis/watcher.go
Outdated
| if seq := mvccValue.KVNemesisSeq.Get(); seq > 0 { | ||
| w.env.Tracker.Add(key.Key, nil, key.Timestamp, seq) | ||
| } | ||
| if err := w.handleValueLocked(ctx, roachpb.Span{Key: key.Key}, mvccValue.Value, nil); err != nil { |
There was a problem hiding this comment.
Could you add a comment on (*SeqTracker).Add explaining that for point writes, we rely on a RangefeedValueHeaderFilter knob to tell us the seqnos of writes as they get emitted from the rangefeed, while for AddSST the emitted rangefeed event itself contains the seqno and so it is added in the watcher?
This is a leaky comment but I think it tells people everything they need to know.
erikgrinaker
force-pushed
the
kvnemesis-addsstable
branch
from
770c480 to
df57466
Compare
|
bors r+ |
|
Build failed: |
|
bors retry |
This patch adds support for `AddSSTable` operations in kvnemesis. It generates SSTs of 1-16 point key/value pairs, using options that make the ingestion MVCC-compliant (`SSTTimestampToRequestTimestamp` and `DisallowConflicts`). It does not yet test ingestion of MVCC range tombstones, since this is not yet used in production code, and to avoid blocking this PR on any range key-related issues. A later PR will address this. `AddSSTable` requests cannot span multiple ranges, so the generator optimistically builds SSTs based on the currently known splits. This may race with concurrent splits causing the request to fail, but the majority of requests will succeed. Release note: None
erikgrinaker
force-pushed
the
kvnemesis-addsstable
branch
from
df57466 to
2660382
Compare
|
Canceled. |
|
bors retry |
|
Build failed (retrying...): |
|
Build succeeded: |
This patch adds support for
AddSSTableoperations in kvnemesis. It generates SSTs of 1-16 point key/value pairs, using options that make the ingestion MVCC-compliant (SSTTimestampToRequestTimestampandDisallowConflicts).It does not yet test ingestion of MVCC range tombstones, since this is not yet used in production code, and to avoid blocking this PR on any range key-related issues. A later PR will address this.
AddSSTablerequests cannot span multiple ranges, so the generator optimistically builds SSTs based on the currently known splits. This may race with concurrent splits causing the request to fail, but the majority of requests will succeed.Resolves #64824.
Release note: None