storage: remove MVCCIterator.Key method #98542
Conversation
|
This change is |
The MVCCIterator interface previously exposed two methods for accessing the current iterator postion as a MVCC key—UnsafeKey and Key. Key() was equivalent of calling UnsafeKey().Clone(). This commit removes the Key() variant, pushing the onus of key copying onto the caller. This reduces the interface surface area, avoids accidental key copying (some of which is addressed within this key), and does not impose any unreasonable burden on callers. Epic; None Informs cockroachdb#82589. Release note: None
jbowens
requested review from
RaduBerinde,
rhu713,
michae2,
erikgrinaker and
a team
and removed request for
a team
There was a problem hiding this comment.
Happy to see this change.
All the changes here look fine, but just curious whether all tests are still using unsafeMVCCIterator (or some equivalent) that will mangle the unsafe key?
Reviewed 33 of 33 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde and @rhu713)
There was a problem hiding this comment.
All the changes here look fine, but just curious whether all tests are still using unsafeMVCCIterator (or some equivalent) that will mangle the unsafe key?
Yeah, it now happens through the storage/pebbleiter.Iterator type which performs the mangling right above the *pebble.Iterator. It may also be worthwhile to perform analogous mangling directly above the top iterator returned from the storage package.
TFTRs!
bors r+
Reviewable status:
|
Build failed: |
|
Failed in UI tests: bors r+ |
|
Build succeeded: |
The MVCCIterator interface previously exposed two methods for accessing the current iterator postion as a MVCC key—UnsafeKey and Key. Key() was equivalent to UnsafeKey().Clone().
This commit removes the Key() variant, pushing the onus of key copying onto the caller. This reduces the interface surface area, avoids accidental key copying (some of which is addressed within this commit), and does not impose any unreasonable burden on callers.
Epic: None
Informs #82589.
Release note: None