internal/rangekey: remove ForeignSSTTransformer #2782
Conversation
|
This change is |
There was a problem hiding this comment.
Do we have metamorphic test coverage that should have caught this?
Is this ForeignSSTTransformer still necessary? I thought that we could use ordinary ingest global seqnum replacement for range keys now?
Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @RahulAggarwal1016)
Tests in cockroachdb#2698 combined with `TestIngestShared` caught that we were not returning range keys in the order expected by other internal iterators when reading from a foreign sstable. This change removes the ForeignSSTTransformer as it's no longer necessary given all range keys are emitted at the ingestion sequence number.
itsbilal
force-pushed
the
foreign-sst-rangekey-sort
branch
from
f9e6a07
to
02ec4a8
Compare
|
@jbowens Yes, metamorphic testing of foreign sstable iterators would have caught this, so #2711 is probably the one to wait for. And you're right, we don't need the coalescing logic anymore. There was one concern around excess rangekeydels/unsets being emitted due to snapshots (because the obsolete key kind logic doesn't really apply here), but I don't think it should functionally change anything given all keys in the sstable will have the same sequence number. I've changed this PR to just remove |
itsbilal
changed the title
|
lgtm |
|
TFTR! |
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion
internal/rangekey/coalesce.go line 412 at r2 (raw file):
fallthrough case 6: if seqNum != base.SeqNumForLevel(f.Level) {
We can now also update the ingestion code path to set normal sequence numbers for shared files (but in an order that respects their levels) and remove base.SeqNumForLevel, base.SeqNumL5, etc, right?
This change updates the virtual sstable range key iterator to hide obsolete range keys for shared ingested files. It does this by reusing the ForeignSSTTransformer that was removed in cockroachdb#2782. This change also cleans up some dangling references to the older `isForeign` way of determining whether to hide obsolete keys, in lieu of the newer isSharedIngested approach to address boomerang shared files. Fixes cockroachdb#3174.
This change updates the virtual sstable range key iterator to hide obsolete range keys for shared ingested files. It does this by reusing the ForeignSSTTransformer that was removed in cockroachdb#2782. This change also cleans up some dangling references to the older `isForeign` way of determining whether to hide obsolete keys, in lieu of the newer isSharedIngested approach to address boomerang shared files. Fixes cockroachdb#3174.
This change updates the virtual sstable range key iterator to hide obsolete range keys for shared ingested files. It does this by reusing the ForeignSSTTransformer that was removed in cockroachdb#2782. This change also cleans up some dangling references to the older `isForeign` way of determining whether to hide obsolete keys, in lieu of the newer isSharedIngested approach to address boomerang shared files. Fixes cockroachdb#3174.
This change updates the virtual sstable range key iterator to hide obsolete range keys for shared ingested files. It does this by reusing the ForeignSSTTransformer that was removed in cockroachdb#2782. This change also cleans up some dangling references to the older `isForeign` way of determining whether to hide obsolete keys, in lieu of the newer isSharedIngested approach to address boomerang shared files. Fixes cockroachdb#3174.
This change updates the virtual sstable range key iterator to hide obsolete range keys for shared ingested files. It does this by reusing the ForeignSSTTransformer that was removed in #2782. This change also cleans up some dangling references to the older `isForeign` way of determining whether to hide obsolete keys, in lieu of the newer isSharedIngested approach to address boomerang shared files. Fixes #3174.
Tests in #2698 combined with
TestIngestSharedcaught that wewere not returning range keys in the order expected by other internal
iterators when reading from a foreign sstable. This change removes
the ForeignSSTTransformer as it's no longer necessary given
all range keys are emitted at the ingestion sequence number.