Add an optional min file size requirement for deletion triggered compaction #13707

cbi42 · 2025-06-17T17:57:16Z

Summary: add the min_file_size parameter to CompactOnDeletionCollector. A file must be at least this size for it to qualify for DTC. This is useful when a user wants to specific a min file size requirement that is larger than the size constraint imposed by the sliding window's deletion_trigger requirement.

Added some comment explaining that the file_size provided to table property collector only includes data blocks and may not be up-to-date. This PR also updates DTC to consider SingleDelete and DeletionWithTimestamp as tombstones.

Test plan:

new unit test for when min_file_size is specified.
existing unit test for when min_file_size is not specified.

facebook-github-bot · 2025-06-17T18:42:20Z

@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

hx235 · 2025-06-18T17:22:26Z

include/rocksdb/utilities/table_properties_collectors.h

@@ -31,7 +31,8 @@ class CompactOnDeletionCollectorFactory
  //     based on deletion ratio.


Maybe also expand this comment to include min_file_size and edit: note the limitation with parallel compression.

accumulated_file_size would be a better name IMO (but not a big deal if we can't change it now due to public API tax)

nit: // entries >= deletion_ratio of the entire file - just to emphasize that this deletion_ratio applies to the whole file (not like how D and N enforces a deletion ratio in a sliding portion of the file)

Added some notes on file size and fixed the comment for nit.

hx235 · 2025-06-18T17:26:06Z

utilities/table_properties_collectors/compact_on_deletion_collector.cc

+    // The file may qualify for compaction based on file size constraints,
+    // even if max_deletion_in_window_ is not updated.
+    if (max_deletion_in_window_ >= deletion_trigger_ &&
+        file_size >= min_file_size_) {
+      need_compaction_ = true;
+    }


I'm still forming the right mental model of this. I feel like this is the same/replace-able by checking the file size and max_deletion_in_window_ (historical highest) >= deletion_trigger_ once in Finish().

Did I miss anything why you have to do the check here?

Oh by the way, global_max_deletions_in_window seems to indicate the historical aspect slightly better.

If we remember the file size, then it can be done in Finish(). There may be cases where we want to know if a file can be marked for compaction before the end of a file (e.g. for early file cutting).

Thanks! I thought "Marked For Compaction" is an info only useful to a file that already finishes writing (so after Finish() is called). What do you mean by early file cutting?

hx235 · 2025-06-18T17:47:41Z

utilities/table_properties_collectors/compact_on_deletion_collector_test.cc

+      std::unique_ptr<TablePropertiesCollector> collector(
+          factory->CreateTablePropertiesCollector(context));
+
+      // Add enough deletions to meet both sliding window and ratio triggers


By "and ratio triggers", I assume you don't mean deletion_ratio but the "deletion ratio" enforced in the sliding window. Can we clarify?

Sorry it's an outdated comment, this case is meant for sliding window only.

hx235 · 2025-06-18T17:51:12Z

include/rocksdb/table_properties.h

+  // @params file_size the current file size. For BlockBasedTable, this
+  //         includes all the data blocks written so far, upto but not including
+  //         the current block being built. With parallel compression, data
+  //         blocks are written async so it depends on the compression progress.


Can be a follow up work: add the CompactOnDeletionCollectorFactory to stress test

options.table_properties_collector_factories.emplace_back( std::make_shared<DbStressTablePropertiesCollectorFactory>());

Thanks, that will be a good onboarding task.

hx235

Looking great - the only real blocking comment is https://github.com/facebook/rocksdb/pull/13707/files#r2155144571 before I can approve.

pdillinger

LGTM

pdillinger · 2025-06-18T20:06:27Z

utilities/table_properties_collectors/compact_on_deletion_collector.cc

@@ -39,8 +42,10 @@ Status CompactOnDeletionCollector::AddUserKey(const Slice& /*key*/,
                                              const Slice& /*value*/,
                                              EntryType type,
                                              SequenceNumber /*seq*/,
-                                              uint64_t /*file_size*/) {
+                                              uint64_t file_size) {
+#ifndef NDEBUG


not strictly required because assert() is a macro

Yes, I was hoping to reduce the memory needed but it won't make a difference given other existing class members.

pdillinger · 2025-06-18T20:09:50Z

utilities/table_properties_collectors/compact_on_deletion_collector_test.cc

+                                      file_size));
+
+      ASSERT_OK(collector->Finish(nullptr));
+      ASSERT_EQ(collector->NeedCompact(), file_size >= kMinFileSize);


Ideally there would be an integration test (as well as the unit test), but I'm not going to go crazy over that for this feature building on existing stuff.

facebook-github-bot · 2025-06-19T02:25:23Z

@cbi42 has updated the pull request. You must reimport the pull request before landing.

cbi42

Thanks for the review

cbi42 · 2025-06-19T00:12:38Z

include/rocksdb/utilities/table_properties_collectors.h

@@ -31,7 +31,8 @@ class CompactOnDeletionCollectorFactory
  //     based on deletion ratio.


Added some notes on file size and fixed the comment for nit.

cbi42 · 2025-06-19T00:13:39Z

include/rocksdb/table_properties.h

+  // @params file_size the current file size. For BlockBasedTable, this
+  //         includes all the data blocks written so far, upto but not including
+  //         the current block being built. With parallel compression, data
+  //         blocks are written async so it depends on the compression progress.


Thanks, that will be a good onboarding task.

cbi42 · 2025-06-19T00:17:12Z

utilities/table_properties_collectors/compact_on_deletion_collector_test.cc

+      std::unique_ptr<TablePropertiesCollector> collector(
+          factory->CreateTablePropertiesCollector(context));
+
+      // Add enough deletions to meet both sliding window and ratio triggers


Sorry it's an outdated comment, this case is meant for sliding window only.

cbi42 · 2025-06-19T00:19:02Z

utilities/table_properties_collectors/compact_on_deletion_collector.cc

@@ -39,8 +42,10 @@ Status CompactOnDeletionCollector::AddUserKey(const Slice& /*key*/,
                                              const Slice& /*value*/,
                                              EntryType type,
                                              SequenceNumber /*seq*/,
-                                              uint64_t /*file_size*/) {
+                                              uint64_t file_size) {
+#ifndef NDEBUG


Yes, I was hoping to reduce the memory needed but it won't make a difference given other existing class members.

facebook-github-bot · 2025-06-19T02:45:03Z

@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

hx235

LGTM Thanks!

cbi42 marked this pull request as ready for review June 17, 2025 17:57

facebook-github-bot added the CLA Signed label Jun 17, 2025

cbi42 requested review from pdillinger and hx235 June 17, 2025 19:59

hx235 reviewed Jun 18, 2025

View reviewed changes

pdillinger approved these changes Jun 18, 2025

View reviewed changes

cbi42 added 4 commits June 18, 2025 19:25

min file size

500edd9

release note

8d35a5c

address review feedback

91e5a62

release note for concurrent vector

9b3abb1

cbi42 force-pushed the min-file-size-dtc branch from 2c23bff to 9b3abb1 Compare June 19, 2025 02:25

cbi42 commented Jun 19, 2025

View reviewed changes

hx235 approved these changes Jun 19, 2025

View reviewed changes

		@@ -31,7 +31,8 @@ class CompactOnDeletionCollectorFactory
		// based on deletion ratio.

Add an optional min file size requirement for deletion triggered compaction #13707

Are you sure you want to change the base?

Add an optional min file size requirement for deletion triggered compaction #13707

Uh oh!

Conversation

cbi42 commented Jun 17, 2025

Uh oh!

facebook-github-bot commented Jun 17, 2025

Uh oh!

hx235 Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hx235 Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hx235 Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hx235 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pdillinger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jun 19, 2025

Uh oh!

cbi42 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jun 19, 2025

Uh oh!

hx235 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hx235 Jun 18, 2025 •

edited

Loading

hx235 Jun 18, 2025 •

edited

Loading

hx235 Jun 18, 2025 •

edited

Loading

hx235 left a comment •

edited

Loading

hx235 left a comment •

edited

Loading