Skip to content

Commit

Permalink
compaction: improve partition estimates for garbage collected sstables
Browse files Browse the repository at this point in the history
When a compaction strategy uses garbage collected sstables to track
expired tombstones, do not use complete partition estimates for them,
instead, use a fraction of it based on the droppable tombstone ratio
estimate.

Fixes #18283

Signed-off-by: Lakshmi Narayanan Sreethar <lakshmi.sreethar@scylladb.com>

Closes #18465

(cherry picked from commit d39adf6)

Closes #18656
  • Loading branch information
lkshminarayanan authored and denesb committed May 14, 2024
1 parent 28d0fc1 commit 4b0c60c
Showing 1 changed file with 8 additions and 1 deletion.
9 changes: 8 additions & 1 deletion compaction/compaction.cc
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,7 @@ class compaction {
// fully expired files, which are skipped, aren't taken into account.
uint64_t _compacting_data_file_size = 0;
uint64_t _estimated_partitions = 0;
double _estimated_droppable_tombstone_ratio = 0;
uint64_t _bloom_filter_checks = 0;
db::replay_position _rp;
encoding_stats_collector _stats_collector;
Expand Down Expand Up @@ -588,7 +589,8 @@ class compaction {
sstable_writer_config cfg = _table_s.configure_writer("garbage_collection");
cfg.run_identifier = gc_run;
cfg.monitor = monitor.get();
auto writer = sst->get_writer(*schema(), partitions_per_sstable(), cfg, get_encoding_stats());
uint64_t estimated_partitions = std::max(1UL, uint64_t(ceil(partitions_per_sstable() * _estimated_droppable_tombstone_ratio)));
auto writer = sst->get_writer(*schema(), estimated_partitions, cfg, get_encoding_stats());
return compaction_writer(std::move(monitor), std::move(writer), std::move(sst));
}

Expand Down Expand Up @@ -707,6 +709,7 @@ class compaction {
auto fully_expired = _table_s.fully_expired_sstables(_sstables, gc_clock::now());
min_max_tracker<api::timestamp_type> timestamp_tracker;

double sum_of_estimated_droppable_tombstone_ratio = 0;
_input_sstable_generations.reserve(_sstables.size());
for (auto& sst : _sstables) {
co_await coroutine::maybe_yield();
Expand All @@ -733,6 +736,8 @@ class compaction {
// for a better estimate for the number of partitions in the merged
// sstable than just adding up the lengths of individual sstables.
_estimated_partitions += sst->get_estimated_key_count();
auto gc_before = sst->get_gc_before_for_drop_estimation(gc_clock::now(), _table_s.get_tombstone_gc_state(), _schema);
sum_of_estimated_droppable_tombstone_ratio += sst->estimate_droppable_tombstone_ratio(gc_before);
_compacting_data_file_size += sst->ondisk_data_size();
// TODO:
// Note that this is not fully correct. Since we might be merging sstables that originated on
Expand All @@ -748,6 +753,8 @@ class compaction {
log_debug("{} out of {} input sstables are fully expired sstables that will not be actually compacted",
_sstables.size() - ssts->size(), _sstables.size());
}
// _estimated_droppable_tombstone_ratio could exceed 1.0 in certain cases, so limit it to 1.0.
_estimated_droppable_tombstone_ratio = std::min(1.0, sum_of_estimated_droppable_tombstone_ratio / ssts->size());

This comment has been minimized.

Copy link
@mykaul

mykaul May 19, 2024

Contributor

@avikivity - this is the line I've referred to in the meeting

This comment has been minimized.

Copy link
@avikivity

avikivity May 19, 2024

Member

By now I forgot the context


_compacting = std::move(ssts);

Expand Down

0 comments on commit 4b0c60c

Please sign in to comment.