-
Notifications
You must be signed in to change notification settings - Fork 477
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
metamorphic: ignore rangedels for deduplicating point prefixes #4280
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @jbowens and @sumeerbhola)
metamorphic/key_manager.go
line 31 at r1 (raw file):
// types. func (m *keyMeta) all(typs ...OpType) bool { return slices.ContainsFunc(m.history, func(i keyHistoryItem) bool {
shouldn't there be a !
here? We're returning true if one of the items in history
is not one of typs
metamorphic/testdata/key_manager
line 505 at r1 (raw file):
[external1 = batch1.NewExternalObj()] [db1.IngestExternalFiles(external1, "a" /* start */, "z" /* end */, "" /* syntheticSuffix */, "" /* syntheticPrefix */)] can singledel on db1: "foo@5"
[nit] add trailing newline
7acd4aa
to
c1cce06
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @RaduBerinde and @sumeerbhola)
metamorphic/key_manager.go
line 31 at r1 (raw file):
Previously, RaduBerinde wrote…
shouldn't there be a
!
here? We're returning true if one of the items inhistory
is not one oftyps
lol thanks, I really confused myself with the repeated negation. I added some local vars that maybe help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @jbowens and @sumeerbhola)
metamorphic/key_manager.go
line 31 at r2 (raw file):
// types. func (m *keyMeta) all(typs ...OpType) bool { anyNotOfProvidedType := slices.ContainsFunc(m.history, func(i keyHistoryItem) bool {
[int] containsOtherTypes
to avoid the double negation.
When constructing an external object, only the first point key with each unique prefix is added to the sstable. The key manager strives to maintain consistency with this behavior to ensure that the it maintains correct key histories after an ingestion of an external object. The key manager however also will discretize range deletions, sometimes creating keyMetas for keys that do not exist as point keys on the associated writer. The presence of these was confusing the key manager's calculation of which keys would end up in the external object. This commit updates the external object calculation to account for the existence of the range deletions. Future work may refactor the tracking of range deletions to track range deletions separately from point keys and as continuous spans. Fix cockroachdb#4267.
c1cce06
to
0e84b9a
Compare
When constructing an external object, only the first point key with each unique prefix is added to the sstable. The key manager strives to maintain consistency with this behavior to ensure that the it maintains correct key histories after an ingestion of an external object. The key manager however also will discretize range deletions, sometimes creating keyMetas for keys that do not exist as point keys on the associated writer. The presence of these was confusing the key manager's calculation of which keys would end up in the external object.
This commit updates the external object calculation to account for the existence of the range deletions. Future work may refactor the tracking of range deletions to track range deletions separately from point keys and as continuous spans.
Fix #4267.