Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ingest: IngestAndExcise should use flushableIngest for ingests #3335

Closed
aadityasondhi opened this issue Feb 22, 2024 · 1 comment · Fixed by #3398
Closed

ingest: IngestAndExcise should use flushableIngest for ingests #3335

aadityasondhi opened this issue Feb 22, 2024 · 1 comment · Fixed by #3398
Assignees
@aadityasondhi
Labels
A-storage T-storage

Comments

@aadityasondhi
Copy link
Contributor

This came up in an internal thread.

When we use IngestAndExcise as default and have an overlap with the memtable, we have to wait for the memtable to flush since we don't use flushable ingest. This is because for the general IngestAndExcise use case, the sstables are from another LSM and do not have a RANGEDEL.

A possible solution sketch (from the discussion):

Option to pass to IngestAndExcise promising that the exciseSpan is also repeated explicitly via RANGEDELs in the sstables. Then we could layer these on top as flushable ingests, and when it came time to actually flush this flushable ingest, we would do the excise, and get the lower read amp.
@itsbilal
Copy link
Member

Note that this issue is about the interim solution of throwing in rangedels with the sstables, and having an option for that. The better solution where we expose rangedels with iterators on the flushable ingests is #2676.

@sumeerbhola sumeerbhola mentioned this issue Mar 5, 2024
Closed
7 tasks
aadityasondhi added a commit to aadityasondhi/pebble that referenced this issue Mar 13, 2024
@aadityasondhi
ingest,compaction: use excise when flushing flushableIngest
557fc30 
This patch ammends the ingest pipeline to allow for certain cases to use
`flushaleIngest` in order to avoid waiting on memtable flushes when
there is overlap. The current use for this is range snapshots in
Cockroach where we include `RANGEDEL`. When we use this path, we also
pass the `exciseSpan` down down into the flushable to then excise at
flush time to still benefit from the use of excise in the ingest path.

Fixes cockroachdb#3335.
aadityasondhi added a commit to aadityasondhi/pebble that referenced this issue Mar 13, 2024
@aadityasondhi
ingest,compaction: use excise when flushing flushableIngest
a59b16a 
This patch amends the ingest pipeline to allow for certain cases to use
`flushaleIngest` in order to avoid waiting on memtable flushes when
there is overlap. The current use for this is range snapshots in
Cockroach where we include `RANGEDEL`. When we use this path, we also
pass the `exciseSpan` down down into the flushable to then excise at
flush time to still benefit from the use of excise in the ingest path.

Fixes cockroachdb#3335.
aadityasondhi added a commit to aadityasondhi/pebble that referenced this issue Mar 13, 2024
@aadityasondhi
ingest,compaction: use excise when flushing flushableIngest
a25a866 
This patch amends the ingest pipeline to allow for certain cases to use
`flushaleIngest` in order to avoid waiting on memtable flushes when
there is overlap. The current use for this is range snapshots in
Cockroach where we include `RANGEDEL`. When we use this path, we also
pass the `exciseSpan` down down into the flushable to then excise at
flush time to still benefit from the use of excise in the ingest path.

Fixes cockroachdb#3335.
@aadityasondhi aadityasondhi mentioned this issue Mar 13, 2024
Merged
aadityasondhi added a commit to aadityasondhi/pebble that referenced this issue Mar 13, 2024
@aadityasondhi
ingest,compaction: use excise when flushing flushableIngest
4060cfa 
This patch amends the ingest pipeline to allow for certain cases to use
`flushaleIngest` in order to avoid waiting on memtable flushes when
there is overlap. The current use for this is range snapshots in
Cockroach where we include `RANGEDEL`. When we use this path, we also
pass the `exciseSpan` down down into the flushable to then excise at
flush time to still benefit from the use of excise in the ingest path.

Fixes cockroachdb#3335.
aadityasondhi added a commit to aadityasondhi/pebble that referenced this issue Mar 13, 2024
@aadityasondhi
ingest,compaction: use excise when flushing flushableIngest
ef18be5 
This patch amends the ingest pipeline to allow for certain cases to use
`flushaleIngest` in order to avoid waiting on memtable flushes when
there is overlap. The current use for this is range snapshots in
Cockroach where we include `RANGEDEL`. When we use this path, we also
pass the `exciseSpan` down down into the flushable to then excise at
flush time to still benefit from the use of excise in the ingest path.

Fixes cockroachdb#3335.
aadityasondhi added a commit to aadityasondhi/pebble that referenced this issue Mar 20, 2024
@aadityasondhi
ingest,compaction: use excise when flushing flushableIngest
796b7d6 
This patch amends the ingest pipeline to allow for certain cases to use
`flushaleIngest` in order to avoid waiting on memtable flushes when
there is overlap. The current use for this is range snapshots in
Cockroach where we include `RANGEDEL`. When we use this path, we also
pass the `exciseSpan` down down into the flushable to then excise at
flush time to still benefit from the use of excise in the ingest path.

Fixes cockroachdb#3335.
aadityasondhi added a commit to aadityasondhi/pebble that referenced this issue Mar 20, 2024
@aadityasondhi
ingest,compaction: use excise when flushing flushableIngest
f3cc528 
This patch amends the ingest pipeline to allow for certain cases to use
`flushaleIngest` in order to avoid waiting on memtable flushes when
there is overlap. The current use for this is range snapshots in
Cockroach where we include `RANGEDEL`. When we use this path, we also
pass the `exciseSpan` down down into the flushable to then excise at
flush time to still benefit from the use of excise in the ingest path.

Fixes cockroachdb#3335.
aadityasondhi added a commit to aadityasondhi/pebble that referenced this issue Mar 20, 2024
@aadityasondhi
ingest,compaction: use excise when flushing flushableIngest
847af93 
This patch amends the ingest pipeline to allow for certain cases to use
`flushaleIngest` in order to avoid waiting on memtable flushes when
there is overlap. The current use for this is range snapshots in
Cockroach where we include `RANGEDEL`. When we use this path, we also
pass the `exciseSpan` down down into the flushable to then excise at
flush time to still benefit from the use of excise in the ingest path.

Fixes cockroachdb#3335.
aadityasondhi added a commit to aadityasondhi/pebble that referenced this issue Mar 20, 2024
@aadityasondhi
ingest,compaction: use excise when flushing flushableIngest
80b7b75 
This patch amends the ingest pipeline to allow for certain cases to use
`flushaleIngest` in order to avoid waiting on memtable flushes when
there is overlap. The current use for this is range snapshots in
Cockroach where we include `RANGEDEL`. When we use this path, we also
pass the `exciseSpan` down down into the flushable to then excise at
flush time to still benefit from the use of excise in the ingest path.

Fixes cockroachdb#3335.
aadityasondhi added a commit to aadityasondhi/pebble that referenced this issue Mar 20, 2024
@aadityasondhi
ingest,compaction: use excise when flushing flushableIngest
ba3ea2a 
This patch amends the ingest pipeline to allow for certain cases to use
`flushaleIngest` in order to avoid waiting on memtable flushes when
there is overlap. The current use for this is range snapshots in
Cockroach where we include `RANGEDEL`. When we use this path, we also
pass the `exciseSpan` down down into the flushable to then excise at
flush time to still benefit from the use of excise in the ingest path.

Fixes cockroachdb#3335.
aadityasondhi added a commit to aadityasondhi/pebble that referenced this issue Mar 20, 2024
@aadityasondhi
ingest,compaction: use excise when flushing flushableIngest
aa2c985 
This patch amends the ingest pipeline to allow for certain cases to use
`flushaleIngest` in order to avoid waiting on memtable flushes when
there is overlap. The current use for this is range snapshots in
Cockroach where we include `RANGEDEL`. When we use this path, we also
pass the `exciseSpan` down down into the flushable to then excise at
flush time to still benefit from the use of excise in the ingest path.

Fixes cockroachdb#3335.
aadityasondhi added a commit to aadityasondhi/pebble that referenced this issue Mar 20, 2024
@aadityasondhi
ingest,compaction: use excise when flushing flushableIngest
91d8118 
This patch amends the ingest pipeline to allow for certain cases to use
`flushaleIngest` in order to avoid waiting on memtable flushes when
there is overlap. The current use for this is range snapshots in
Cockroach where we include `RANGEDEL`. When we use this path, we also
pass the `exciseSpan` down down into the flushable to then excise at
flush time to still benefit from the use of excise in the ingest path.

Fixes cockroachdb#3335.
aadityasondhi added a commit to aadityasondhi/pebble that referenced this issue Mar 20, 2024
@aadityasondhi
ingest,compaction: use excise when flushing flushableIngest
fbfa1d2 
This patch amends the ingest pipeline to allow for certain cases to use
`flushaleIngest` in order to avoid waiting on memtable flushes when
there is overlap. The current use for this is range snapshots in
Cockroach where we include `RANGEDEL`. When we use this path, we also
pass the `exciseSpan` down down into the flushable to then excise at
flush time to still benefit from the use of excise in the ingest path.

Fixes cockroachdb#3335.
aadityasondhi added a commit that referenced this issue Mar 21, 2024
@aadityasondhi
ingest,compaction: use excise when flushing flushableIngest
8a097e8 
This patch amends the ingest pipeline to allow for certain cases to use
`flushaleIngest` in order to avoid waiting on memtable flushes when
there is overlap. The current use for this is range snapshots in
Cockroach where we include `RANGEDEL`. When we use this path, we also
pass the `exciseSpan` down down into the flushable to then excise at
flush time to still benefit from the use of excise in the ingest path.

Fixes #3335.
@aadityasondhi aadityasondhi mentioned this issue Mar 26, 2024
Merged
aadityasondhi added a commit to aadityasondhi/cockroach that referenced this issue Mar 26, 2024
@aadityasondhi
storage,ingest: enable ingestion optimization using flushableIngest
df7ba37 
In cockroachdb/pebble#3398, we introduced an
optimization in pebble where we could use flushableIngests with excises
for ingesting SSTs with RangeDels and RangeKeyDels.

This patch turns this optimization on using
`sstContainsExciseTombstone`.

Informs cockroachdb/pebble#3335.

Release note: None
aadityasondhi added a commit to aadityasondhi/cockroach that referenced this issue Mar 27, 2024
@aadityasondhi
storage,ingest: enable ingestion optimization using flushableIngest
9357082 
In cockroachdb/pebble#3398, we introduced an
optimization in pebble where we could use flushableIngests with excises
for ingesting SSTs with RangeDels and RangeKeyDels.

This patch turns this optimization on using
`sstContainsExciseTombstone`.

Informs cockroachdb/pebble#3335.

Release note: None
craig bot pushed a commit to cockroachdb/cockroach that referenced this issue Mar 27, 2024
@aadityasondhi @erikgrinaker @rafiss
Merge #121138 #121164 #121212
87ed0b2 
121138: storage,ingest: enable ingestion optimization using flushableIngest r=aadityasondhi a=aadityasondhi

In cockroachdb/pebble#3398, we introduced an optimization in pebble where we could use flushableIngests with excises for ingesting SSTs with RangeDels and RangeKeyDels.

This patch turns this optimization on using
`sstContainsExciseTombstone`.

Informs cockroachdb/pebble#3335.

Release note: None

121164: Revert "rangefeed: unconditionally use rangefeed scheduler" r=erikgrinaker a=erikgrinaker

This reverts 3/4 commits from #114410, only leaving 1dbdbe9 which enabled the scheduler by default.

There was significant code drift in tests, but I think I've been able to address the conflicts.

This revert does not integrate with the changes from #120347, so the legacy rangefeed processor will not use the improved memory accounting. We still shouldn't leak budget allocations, since the processor implementations are entirely independent, but it's worth having a second look to make sure the registry (which is used by both processors) don't make assumptions about this.

Touches #121054.
Epic: none.
Release note: the following release note from #114410 should be reverted/ignored: "the setting kv.rangefeed.scheduler.enabled has been removed, as the rangefeed scheduler is now unconditionally enabled".

121212: security: wrap error when parsing DN r=rafiss a=rafiss

Epic: CRDB-34126
Release note: None

Co-authored-by: Aaditya Sondhi <[email protected]>
Co-authored-by: Erik Grinaker <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>
@jbowens jbowens moved this to Done in [Deprecated] Storage Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@aadityasondhi aadityasondhi

Labels
A-storage T-storage
Projects
[Deprecated] Storage
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

ingest,compaction: use excise when flushing flushableIngest aadityasondhi/pebble
2 participants
@itsbilal @aadityasondhi