Support periodic compaction in universal compaction #5970

siying · 2019-10-25T21:54:07Z

Summary:
Previously, periodic compaction is not supported in universal compaction. Add the support using following approach: if any file is marked as qualified for periodid compaction, trigger a full compaction. If a full compaction is prevented by files being compacted, try to compact the higher levels than files currently being compacted. If in this way we can only compact the last sorted run and none of the file to be compacted qualifies for periodic compaction, skip the compact. This is to prevent the same single level compaction from being executed again and again.

Test Plan: Add several test cases.

facebook-github-bot

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

facebook-github-bot · 2019-10-29T18:21:42Z

@siying has updated the pull request. Re-import the pull request

facebook-github-bot · 2019-10-29T23:32:10Z

@siying has updated the pull request. Re-import the pull request

facebook-github-bot · 2019-10-30T00:29:52Z

@siying has updated the pull request. Re-import the pull request

sagar0 · 2019-10-30T21:11:20Z

db/compaction/compaction_picker_test.cc

@@ -499,6 +499,120 @@ TEST_F(CompactionPickerTest, AllowsTrivialMoveUniversal) {
  ASSERT_TRUE(compaction->is_trivial_move());
 }

+TEST_F(CompactionPickerTest, UniversalPeriodiCompaction1) {


s/PeriodiCompaction/PeriodicCompaction/g

sagar0 · 2019-10-30T21:20:43Z

db/version_set.h

@@ -312,6 +312,10 @@ class VersionStorageInfo {
    return files_marked_for_periodic_compaction_;
  }

+  void AddFileMarkedForPeriodiCompaction(int level, FileMetaData* f) {


Should this be prepended with "TEST_" prefix ?

sagar0 · 2019-10-30T21:24:39Z

db/db_universal_compaction_test.cc

@@ -2150,6 +2150,72 @@ TEST_F(DBTestUniversalDeleteTrigCompaction, IngestBehind) {
  ASSERT_GT(NumTableFilesAtLevel(5), 0);
 }

+TEST_F(DBTestUniversalDeleteTrigCompaction, PeriodicCompaction) {


I think this test should be under "DBTestUniversalCompaction" instead of "DBTestUniversalDeleteTrigCompaction".
However, the test code itself looks and it is easy to follow.

It feels very odd but non existing test under DBTestUniversalDeleteTrigCompaction is related to delete triggered compaction. I'll just rename this class name to DBTestUniversalCompaction2.

sagar0 · 2019-10-30T21:29:18Z

db/db_universal_compaction_test.cc

+  ASSERT_EQ(0, start_level);
+  ASSERT_EQ(4, output_level);
+
+  // Case 2: Oldest compacted file exccceeds periodic compaction threshold


s/exccceeds/exceeds/

sagar0 · 2019-10-30T21:40:11Z

db/compaction/compaction_picker_universal.cc

+  for (size_t i = 0; i < inputs.size(); ++i) {
+    inputs[i].level = start_level + static_cast<int>(i);
+  }
+  // We always compact all the files, so always compress.


nit: No way related to this PR:
I know this comment is from the original code, but I don't understand what does "so always compress" mean here, and the reason for it. May be this needs to be fixed or elaborated if possible, either in this or a later PR.

Is it talking about enable_compression always being set to true for Compaction() when getting the compression type and options, on lines 1014 and 1016?

I don't remember either. Reading the code, it seems to satisfy option mutable_cf_options_.compaction_options_universal.compression_size_percent. Full compaction is always compressed there. Will add a comment.

sagar0 · 2019-10-30T21:53:55Z

db/compaction/compaction_picker_universal.cc

+    }
+  }
+
+  Compaction* c = PickCompactionToOldest(start_index,


The logic appears simple, but are you not concerned that it might increase write amplification by triggering a full compaction most of the times a periodic compaction is picked?

This is why I chose this way: in universal compaction, the largest file is almost always generated by a full compaction so it's almost always the oldest. So it's pretty uncommon if a periodic compaction doesn't include the oldest file. If we include the oldest file, it makes sense to do a full compaction, because we already compact a big portion of the data. Does it make sense?

riversand963

Mostly LGTM with a few minor comments.

riversand963 · 2019-10-30T22:15:19Z

db/compaction/compaction_picker_universal.cc

@@ -90,6 +90,19 @@ class UniversalCompactionBuilder {

  Compaction* PickDeleteTriggeredCompaction();

+  // Form a compaction from the sorted run indicated by start_index to the
+  // oldest sorted run.
+  // The caller is responsbilt to make sure that those files are not in


s/responsbilt to make/responsible for making/

riversand963 · 2019-10-30T22:17:34Z

db/compaction/compaction_picker_universal.cc

-  if (sorted_runs_.size() >=
-      static_cast<size_t>(
-          mutable_cf_options_.level0_file_num_compaction_trigger)) {
+  if (!vstorage_->FilesMarkedForPeriodicCompaction().empty()) {


Does periodic compaction have higher priority than size-amp-based compaction?

I think it should be the case, as periodic compaction is a hard requirement. Is it the case in the code?

Summary: Previously, periodid compaction is not supported in universal compaction. Add the support using following approach: if any file is marked as qualified for periodid compaction, trigger a full compaction. If a full compaction is prevented by files being compacted, try to compact the higher levels than files currently being compacted. If in this way we can only compact the last level and none of the file at the level qualifies for periodic compaction, skip the compact. This is to prevent the same single level compaction from being executed again and again. Test Plan: Add several test cases.

facebook-github-bot · 2019-10-31T00:23:43Z

@siying has updated the pull request. Re-import the pull request

facebook-github-bot

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

sagar0

lgtm. Thanks!

facebook-github-bot · 2019-10-31T19:40:13Z

This pull request has been merged in aa6f7d0.

Summary: Previously, periodic compaction is not supported in universal compaction. Add the support using following approach: if any file is marked as qualified for periodid compaction, trigger a full compaction. If a full compaction is prevented by files being compacted, try to compact the higher levels than files currently being compacted. If in this way we can only compact the last sorted run and none of the file to be compacted qualifies for periodic compaction, skip the compact. This is to prevent the same single level compaction from being executed again and again. Pull Request resolved: facebook#5970 Test Plan: Add several test cases. Differential Revision: D18147097 fbshipit-source-id: 8ecc308154d9aca96fb192c51fbceba3947550c1

Summary: `options.ttl` is now supported in universal compaction, similar to how periodic compactions are implemented in PR #5970 . Setting `options.ttl` will simply set `options.periodic_compaction_seconds` to execute the periodic compactions code path. Discarded PR #4749 in lieu of this. This is a short term work-around/hack of falling back to periodic compactions when ttl is set. Pull Request resolved: #6071 Test Plan: Added a unit test. Differential Revision: D18668336 Pulled By: sagar0 fbshipit-source-id: e75f5b81ba949f77ef9eff05e44bb1c757f58612

Summary: `options.ttl` is now supported in universal compaction, similar to how periodic compactions are implemented in PR facebook#5970 . Setting `options.ttl` will simply set `options.periodic_compaction_seconds` to execute the periodic compactions code path. Discarded PR facebook#4749 in lieu of this. This is a short term work-around/hack of falling back to periodic compactions when ttl is set. Pull Request resolved: facebook#6071 Test Plan: Added a unit test. Differential Revision: D18668336 Pulled By: sagar0 fbshipit-source-id: e75f5b81ba949f77ef9eff05e44bb1c757f58612

Summary: the comment for option `periodic_compaction_seconds` only mentions support for Leveled and FIFO compaction, while the implementation supports all compaction styles after #5970. This PR updates comment to reflect this. Pull Request resolved: #11227 Reviewed By: ajkr Differential Revision: D43325046 Pulled By: cbi42 fbshipit-source-id: 2364dcb5a01cd098ad52c818fe10d621445e2188

siying requested review from sagar0 and riversand963 October 25, 2019 21:54

facebook-github-bot added the CLA Signed label Oct 25, 2019

facebook-github-bot reviewed Oct 25, 2019

View reviewed changes

sagar0 reviewed Oct 30, 2019

View reviewed changes

riversand963 approved these changes Oct 30, 2019

View reviewed changes

siying added 6 commits October 30, 2019 17:20

Fix sign comparisons.

c28fbe3

Update L0-only case.

601fbfc

Add comments to new functions.

0dd6f41

Address comments.

5c36948

Update HISTORY.md

bb7c10d

siying force-pushed the univ_period3 branch from 9452e52 to bb7c10d Compare October 31, 2019 00:23

facebook-github-bot reviewed Oct 31, 2019

View reviewed changes

sagar0 approved these changes Oct 31, 2019

View reviewed changes

facebook-github-bot closed this in aa6f7d0 Oct 31, 2019

facebook-github-bot added the Merged label Oct 31, 2019

sagar0 mentioned this pull request Nov 22, 2019

Support ttl in Universal Compaction #6071

Closed

cbi42 mentioned this pull request Feb 15, 2023

Fix comment for option periodic_compaction_seconds #11227

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support periodic compaction in universal compaction #5970

Support periodic compaction in universal compaction #5970

siying commented Oct 25, 2019 •

edited

Loading

facebook-github-bot left a comment

facebook-github-bot commented Oct 29, 2019

facebook-github-bot commented Oct 29, 2019

facebook-github-bot commented Oct 30, 2019

sagar0 Oct 30, 2019

sagar0 Oct 30, 2019

sagar0 Oct 30, 2019

siying Oct 31, 2019

sagar0 Oct 30, 2019

sagar0 Oct 30, 2019

siying Oct 31, 2019

sagar0 Oct 30, 2019 •

edited

Loading

siying Oct 30, 2019

riversand963 left a comment

riversand963 Oct 30, 2019

riversand963 Oct 30, 2019

siying Oct 30, 2019

facebook-github-bot commented Oct 31, 2019

facebook-github-bot left a comment

sagar0 left a comment

facebook-github-bot commented Oct 31, 2019

Support periodic compaction in universal compaction #5970

Support periodic compaction in universal compaction #5970

Conversation

siying commented Oct 25, 2019 • edited Loading

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 29, 2019

facebook-github-bot commented Oct 29, 2019

facebook-github-bot commented Oct 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagar0 Oct 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riversand963 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 31, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

sagar0 left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 31, 2019

siying commented Oct 25, 2019 •

edited

Loading

sagar0 Oct 30, 2019 •

edited

Loading