Drain unnecessary levels when `level_compaction_dynamic_level_bytes=true` #11340

cbi42 · 2023-03-30T21:51:57Z

Summary: When a user migrates to level compaction + level_compaction_dynamic_level_bytes=true, or when a DB shrinks, there can be unnecessary levels in the DB. Before this PR, this is no way to remove these levels except a manual compaction. These extra unnecessary levels make it harder to guarantee max_bytes_for_level_multiplier and can cause extra space amp. This PR boosts compaction score for these levels to allow RocksDB to automatically drain these levels. Together with #11321, this makes migration to level_compaction_dynamic_level_bytes=true automatic without needing user to do a one time full manual compaction. Credit: this PR is modified from #3921.

Test plan:

New unit tests
python3 tools/db_crashtest.py whitebox --simple which randomly sets level_compaction_dynamic_level_bytes in each run.

facebook-github-bot · 2023-03-31T00:13:08Z

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

facebook-github-bot · 2023-03-31T18:57:11Z

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

facebook-github-bot · 2023-03-31T19:31:31Z

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

ajkr

Great PR, thanks!

ajkr · 2023-04-03T20:32:21Z

db/compaction/tiered_compaction_test.cc

+  // Otherwise L5 will always be eligible for compaction and
+  // WaitForCompact(true) below will be stuck waiting.


Waiting means sleeping, not infinitely rewriting L5, right?

There will be infinite rewrite of L5 since it always has a eligible compaction score but nothing is written to the last level during each compaction of L5. This causes the WaitForCompact(true) in this unit test to wait forever.

Hm that isn't ideal. We should see if there's a way to not pick a draining compaction when the output level is not necessarily lower.

Done, I've updated the lowest_unnecessary_level_ logic to exclude penultimate level with per key placement. Note that this problem may still exist in general with level compaction + per key placement. It can happen when penultimate level is over target size but with all hot data.

ajkr · 2023-04-03T20:47:30Z

db/db_compaction_test.cc

-  options.level_compaction_dynamic_level_bytes = true;
-  Reopen(options);
-  // note that last level (L6) should be empty
-  ASSERT_EQ("1,0,0,1,2", FilesPerLevel());
-  verify_db();
-
-  // turning the options on and off should both be safe
-  options.level_compaction_dynamic_level_bytes = false;
-  Reopen(options);
-  MoveFilesToLevel(1);
-  ASSERT_EQ("0,1,0,1,2", FilesPerLevel());
-  verify_db();
-
-  // newly flushed file is also pushed down
-  options.level_compaction_dynamic_level_bytes = true;
-  Reopen(options);
-  ASSERT_EQ("0,0,1,1,2", FilesPerLevel());
-  verify_db();
-
-  // files will be pushed down to last level (L6)
-  options.allow_ingest_behind = false;
-  Reopen(options);
-  ASSERT_EQ("0,0,0,1,1,2", FilesPerLevel());


Is this deleted because it's difficult to adapt? Maybe the assertions can just verify the number of files in the bottom level (L5 since num_levels=6) is zero before the final Reopen(), and nonzero after it.

I did find it difficult to adapt since there is some unnecessary level in the LSM which can be drained in the background. I could not just disable auto compaction since it would also disallow trivially moving files down the LSM during DB open. However, I eventually found out a better reason to remove the test :) Otherwise, I should probably just verify the number of last level as you suggested.

Yeah, IIRC there was a comment elsewhere in this PR referring to the better reason - but allow_ingest_behind feels so close to working with leveled compaction that we might as well start testing it when convenient and when there's known risks. Maybe the test can say support is not officially released since on rare occasions people infer what's supported by looking at test expectations

just verify the number of files in the bottom level (L5 since num_levels=6) is zero before the final Reopen(), and nonzero after it.

I tried fixing this, but realize this does not work (without adding support for level compaction + ingest behind) as the second last level can become unnecessary and compacted down even when the last level is reserve for ingest behind. Making it work requires more changes that almost feels like supporting allow_ingest_behind with level compaction in this PR. I filed a task and plan to follow up with a separate PR for official support.

db/db_compaction_test.cc

facebook-github-bot · 2023-04-04T06:33:19Z

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

cbi42

Thanks for the review!

cbi42 · 2023-04-04T04:03:03Z

db/compaction/tiered_compaction_test.cc

+  // Otherwise L5 will always be eligible for compaction and
+  // WaitForCompact(true) below will be stuck waiting.


Done, I've updated the lowest_unnecessary_level_ logic to exclude penultimate level with per key placement. Note that this problem may still exist in general with level compaction + per key placement. It can happen when penultimate level is over target size but with all hot data.

cbi42 · 2023-04-04T06:39:03Z

db/db_compaction_test.cc

-  options.level_compaction_dynamic_level_bytes = true;
-  Reopen(options);
-  // note that last level (L6) should be empty
-  ASSERT_EQ("1,0,0,1,2", FilesPerLevel());
-  verify_db();
-
-  // turning the options on and off should both be safe
-  options.level_compaction_dynamic_level_bytes = false;
-  Reopen(options);
-  MoveFilesToLevel(1);
-  ASSERT_EQ("0,1,0,1,2", FilesPerLevel());
-  verify_db();
-
-  // newly flushed file is also pushed down
-  options.level_compaction_dynamic_level_bytes = true;
-  Reopen(options);
-  ASSERT_EQ("0,0,1,1,2", FilesPerLevel());
-  verify_db();
-
-  // files will be pushed down to last level (L6)
-  options.allow_ingest_behind = false;
-  Reopen(options);
-  ASSERT_EQ("0,0,0,1,1,2", FilesPerLevel());


just verify the number of files in the bottom level (L5 since num_levels=6) is zero before the final Reopen(), and nonzero after it.

I tried fixing this, but realize this does not work (without adding support for level compaction + ingest behind) as the second last level can become unnecessary and compacted down even when the last level is reserve for ingest behind. Making it work requires more changes that almost feels like supporting allow_ingest_behind with level compaction in this PR. I filed a task and plan to follow up with a separate PR for official support.

facebook-github-bot · 2023-04-04T06:42:33Z

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

facebook-github-bot · 2023-04-04T18:53:30Z

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

facebook-github-bot · 2023-04-04T18:54:16Z

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

facebook-github-bot · 2023-04-05T15:42:55Z

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

facebook-github-bot · 2023-04-05T17:24:57Z

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

facebook-github-bot · 2023-04-06T17:09:32Z

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

facebook-github-bot · 2023-04-06T17:09:57Z

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

facebook-github-bot · 2023-04-06T18:24:23Z

@cbi42 merged this pull request in b3c43a5.

…11525) Summary: after #11321 and #11340 (both included in RocksDB v8.2), migration from `level_compaction_dynamic_level_bytes=false` to `level_compaction_dynamic_level_bytes=true` is automatic by RocksDB and requires no manual compaction from user. Making the option true by default as it has several advantages: 1. better space amplification guarantee (a more stable LSM shape). 2. compaction is more adaptive to write traffic. 3. automatic draining of unneeded levels. Wiki is updated with more detail: https://github.com/facebook/rocksdb/wiki/Leveled-Compaction#option-level_compaction_dynamic_level_bytes-and-levels-target-size. The PR mostly contains fixes for unit tests as they assumed `level_compaction_dynamic_level_bytes=false`. Most notable change is commit f742be3 and b1928e4 which override the default option in DBTestBase to still set `level_compaction_dynamic_level_bytes=false` by default. This helps to reduce the change needed for unit tests. I think this default option override in unit tests is okay since the behavior of `level_compaction_dynamic_level_bytes=true` is tested by explicitly setting this option. Also, `level_compaction_dynamic_level_bytes=false` may be more desired in unit tests as it makes it easier to create a desired LSM shape. Comment for option `level_compaction_dynamic_level_bytes` is updated to reflect this change and change made in #10057. Pull Request resolved: #11525 Test Plan: `make -j32 J=32 check` several times to try to catch flaky tests due to this option change. Reviewed By: ajkr Differential Revision: D46654256 Pulled By: cbi42 fbshipit-source-id: 6b5827dae124f6f1fdc8cca2ac6f6fcd878830e1

…rue` (facebook#11340) Summary: When a user migrates to level compaction + `level_compaction_dynamic_level_bytes=true`, or when a DB shrinks, there can be unnecessary levels in the DB. Before this PR, this is no way to remove these levels except a manual compaction. These extra unnecessary levels make it harder to guarantee max_bytes_for_level_multiplier and can cause extra space amp. This PR boosts compaction score for these levels to allow RocksDB to automatically drain these levels. Together with facebook#11321, this makes migration to `level_compaction_dynamic_level_bytes=true` automatic without needing user to do a one time full manual compaction. Credit: this PR is modified from facebook#3921. Pull Request resolved: facebook#11340 Test Plan: - New unit tests - `python3 tools/db_crashtest.py whitebox --simple` which randomly sets level_compaction_dynamic_level_bytes in each run. Reviewed By: ajkr Differential Revision: D44563884 Pulled By: cbi42 fbshipit-source-id: e20d3620bd73dff22be18c5a91a07f340740bcc8 Signed-off-by: tabokie <[email protected]>

facebook-github-bot added the CLA Signed label Mar 30, 2023

ajkr mentioned this pull request Mar 31, 2023

Drain unnecessary middle levels #3921

Closed

cbi42 requested a review from ajkr March 31, 2023 18:57

ajkr approved these changes Apr 3, 2023

View reviewed changes

cbi42 commented Apr 4, 2023

View reviewed changes

cbi42 force-pushed the drain-unnecessary-levels branch from ed0e801 to 697e66f Compare April 4, 2023 18:53

cbi42 force-pushed the drain-unnecessary-levels branch from 697e66f to 91d0486 Compare April 5, 2023 15:42

cbi42 added 8 commits April 6, 2023 10:08

Drain unnecessary levels

2d9961c

Update option

c0ef802

reminder that level + ingest behind is not supported

97ab430

fix test due to level can be drained now

e6d2272

Check status

4d106df

remove unnecessary check

9136d96

do not drain penultimate level with per_key_placement

1b2ea5e

HISTORY.md

ddb7479

cbi42 force-pushed the drain-unnecessary-levels branch from 91d0486 to ddb7479 Compare April 6, 2023 17:09

facebook-github-bot closed this in b3c43a5 Apr 6, 2023

facebook-github-bot added the Merged label Apr 6, 2023

cbi42 mentioned this pull request Apr 7, 2023

Initialize lowest_unnecessary_level_ in VersionStorageInfo constructor #11359

Closed

cbi42 mentioned this pull request Jun 9, 2023

Make option level_compaction_dynamic_level_bytes true by default #11525

Closed

cbi42 mentioned this pull request Jun 26, 2023

Is the behavior of ingest sst the same between setting dynamic level base and default #11561

Closed

tabokie mentioned this pull request Aug 14, 2023

cherry-pick compaction enhancements tikv/rocksdb#345

Closed

igorcanadi mentioned this pull request Jan 17, 2024

[SYS-6913] Upgrade RocksDB-Cloud to 8.9.1 rockset/rocksdb-cloud#315

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drain unnecessary levels when `level_compaction_dynamic_level_bytes=true` #11340

Drain unnecessary levels when `level_compaction_dynamic_level_bytes=true` #11340

cbi42 commented Mar 30, 2023 •

edited

Loading

facebook-github-bot commented Mar 31, 2023

facebook-github-bot commented Mar 31, 2023

facebook-github-bot commented Mar 31, 2023

ajkr left a comment

ajkr Apr 3, 2023

cbi42 Apr 4, 2023 •

edited

Loading

ajkr Apr 4, 2023

cbi42 Apr 4, 2023

ajkr Apr 3, 2023

cbi42 Apr 4, 2023

ajkr Apr 4, 2023

cbi42 Apr 4, 2023

facebook-github-bot commented Apr 4, 2023

cbi42 left a comment

cbi42 Apr 4, 2023

cbi42 Apr 4, 2023

facebook-github-bot commented Apr 4, 2023

facebook-github-bot commented Apr 4, 2023

facebook-github-bot commented Apr 4, 2023

facebook-github-bot commented Apr 5, 2023

facebook-github-bot commented Apr 5, 2023

facebook-github-bot commented Apr 6, 2023

facebook-github-bot commented Apr 6, 2023

facebook-github-bot commented Apr 6, 2023

		// Otherwise L5 will always be eligible for compaction and
		// WaitForCompact(true) below will be stuck waiting.

Drain unnecessary levels when level_compaction_dynamic_level_bytes=true #11340

Drain unnecessary levels when level_compaction_dynamic_level_bytes=true #11340

Conversation

cbi42 commented Mar 30, 2023 • edited Loading

facebook-github-bot commented Mar 31, 2023

facebook-github-bot commented Mar 31, 2023

facebook-github-bot commented Mar 31, 2023

ajkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbi42 Apr 4, 2023 • edited Loading

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

facebook-github-bot commented Apr 4, 2023

cbi42 left a 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 Apr 4, 2023

facebook-github-bot commented Apr 4, 2023

facebook-github-bot commented Apr 4, 2023

facebook-github-bot commented Apr 5, 2023

facebook-github-bot commented Apr 5, 2023

facebook-github-bot commented Apr 6, 2023

facebook-github-bot commented Apr 6, 2023

facebook-github-bot commented Apr 6, 2023

Drain unnecessary levels when `level_compaction_dynamic_level_bytes=true` #11340

Drain unnecessary levels when `level_compaction_dynamic_level_bytes=true` #11340

cbi42 commented Mar 30, 2023 •

edited

Loading

cbi42 Apr 4, 2023 •

edited

Loading