Mark more files for periodic compaction during offpeak #12031

jaykorean · 2023-10-30T23:47:22Z

Summary

The struct previously named OffpeakTimeInfo has been renamed to OffpeakTimeOption to indicate that it's a user-configurable option. Additionally, a new struct, OffpeakTimeInfo, has been introduced, which includes two fields: is_now_offpeak and seconds_till_next_offpeak_start. This change prevents the need to parse the daily_offpeak_time_utc string twice.
It's worth noting that we may consider adding more fields to the OffpeakTimeInfo struct, such as elapsed_seconds and total_seconds, as needed for further optimization.
Within VersionStorageInfo::ComputeFilesMarkedForPeriodicCompaction(), we've adjusted the allowed_time_limit to include files that are expected to expire by the next offpeak start.
We might explore further optimizations, such as evenly distributing files to mark during offpeak hours, if the initial approach results in marking too many files simultaneously during the first scoring in offpeak hours. The primary objective of this PR is to prevent periodic compactions during non-offpeak hours when offpeak hours are configured. We'll start with this straightforward solution and assess whether it suffices for now.

Test Plan

Unit Tests added

DBCompactionTest::LevelPeriodicCompactionOffpeak for Leveled
DBTestUniversalCompaction2::PeriodicCompaction for Universal

facebook-github-bot · 2023-11-01T20:44:20Z

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

facebook-github-bot · 2023-11-01T21:42:50Z

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

pdillinger

AFAIK, the implementation is fine. The tests seem verbose and perhaps not well controlled (changing one variable at a time).

db/db_universal_compaction_test.cc

pdillinger · 2023-11-02T17:05:33Z

db/db_universal_compaction_test.cc

@@ -2230,6 +2231,209 @@ TEST_F(DBTestUniversalCompaction2, PeriodicCompaction) {
  ASSERT_EQ(4, output_level);
 }

+TEST_F(DBTestUniversalCompaction2, PeriodicCompactionOffpeak) {


Is there a significant difference in universal and leveled behavior here? It seems like a lot of duplication (more or less).

From the perspective of picking files based on their age, no. But just wanted to have the coverage on both compaction types.

IMHO, code duplication in tests (more or less) is a greater sin than keeping tests neatly organized by feature. Features interact, so there's no way to be absolutely clean in organization--assuming your tests are interesting and not just a rehash of the implementation; you just put things some place reasonable and respect the greater threats to maintenance burden such as verbosity / unnecessary repetition.

It would probably be different if compaction strategies were pluggable components, but they are deeply integrated.

I appreciate the narrative comments to help future engineers to understand what is being tested and how. Clarity in the "what & why" of a test is most important. :) (Sometimes it's obvious: feature X has its documented effect when turned on and does not when it's turned off.)

Simplified the test for Leveled one, added comment and addressed the feedback in the Universal one.

db/db_compaction_test.cc

cbi42

Do we plan to extend the logic to TTL compaction too?

db/version_set.cc

options/offpeak_time_info.cc

cbi42 · 2023-11-02T17:12:41Z

db/db_universal_compaction_test.cc

+  Random rnd(test::RandomSeed());
+  int days = rnd.Uniform(100);
+
+  int periodic_compactions = 0;


Probably will need atomic_int.

Doesn't compaction picking happen while holding the DB mutex? If so, there would only be a race with the test code if there's a race in executing the periodic compactions.

Yes, compaction picking happens while holding the db mutex. I was worried that the test code can access periodic_compactions while compaction picking is happening. That seems to be not the case. I guess if TSAN complains about it, it's fine too and it means the test has a bug.

I will leave it without atomic and see what TSAN says.

jaykorean · 2023-11-02T17:43:06Z

@cbi42 Regarding the comment on TTL, no, not at the moment since we are going to start with Zippy first which is Universal. But it's definitely something that we can consider adding in the later PRs.

…nstead of clock

facebook-github-bot · 2023-11-03T17:40:51Z

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

facebook-github-bot · 2023-11-03T17:41:27Z

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

facebook-github-bot · 2023-11-03T17:59:30Z

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

facebook-github-bot · 2023-11-03T18:05:26Z

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

facebook-github-bot · 2023-11-03T20:53:11Z

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

facebook-github-bot · 2023-11-03T21:02:16Z

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

facebook-github-bot · 2023-11-03T21:07:45Z

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

facebook-github-bot · 2023-11-03T21:12:54Z

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

facebook-github-bot · 2023-11-03T21:13:25Z

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

facebook-github-bot · 2023-11-03T21:28:51Z

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

facebook-github-bot · 2023-11-03T21:30:55Z

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

facebook-github-bot · 2023-11-03T21:38:15Z

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

facebook-github-bot · 2023-11-03T22:29:23Z

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

facebook-github-bot · 2023-11-03T22:30:57Z

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

facebook-github-bot · 2023-11-03T22:32:30Z

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

facebook-github-bot · 2023-11-03T22:39:13Z

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

cbi42

LGTM

facebook-github-bot · 2023-11-06T19:50:05Z

@jaykorean merged this pull request in 2dab137.

sakae-nakajima · 2024-11-10T18:49:31Z

@jaykorean , thanks for the change. I'd like to try this option in Java application. How can we set this? I see no options in Options nor DBOptions. Thanks

jaykorean · 2024-11-15T04:35:40Z

@sakae-nakajima The option to enable this feature is not exposed in Java API yet. Feel free to try adding it yourself and add me as a reviewer. I'd be more than happy to review the PR. You may also want to try reaching out to the Java API contributors (@adamretter, @alanpaxton and more) for help.

Or I will try finding some time later in December or early next year to add it.

adamretter · 2025-01-06T15:45:23Z

@jaykorean @sakae-nakajima my colleague @alanpaxton has implemented this here - #13148 it is just waiting to be merged by the FB team?

jaykorean · 2025-01-06T19:10:36Z

@adamretter I must have missed it. Let me review and import this to the internal repo. Thanks for the heads up!

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

jaykorean force-pushed the periodic_compaction_during_offpeak branch 4 times, most recently from d07f183 to 16afcb0 Compare November 1, 2023 20:00

jaykorean requested review from pdillinger and cbi42 November 1, 2023 20:38

jaykorean marked this pull request as ready for review November 1, 2023 20:38

pdillinger reviewed Nov 2, 2023

View reviewed changes

cbi42 reviewed Nov 2, 2023

View reviewed changes

jaykorean added 6 commits November 3, 2023 10:40

Mark more files for periodic compaction during offpeak

9e66035

DBOptionsTest::OffpeakTimes updated

ca0c01a

Add Test

8ba5a9c

Fix test

7c78db4

Fix linter

5ce100d

Feedback - parse only when the options is set and take current_time i…

764c0cb

…nstead of clock

jaykorean force-pushed the periodic_compaction_during_offpeak branch from 26c4fa9 to 4e73db6 Compare November 3, 2023 17:40

jaykorean force-pushed the periodic_compaction_during_offpeak branch from 4e73db6 to a4944f3 Compare November 3, 2023 17:41

jaykorean force-pushed the periodic_compaction_during_offpeak branch from a4944f3 to 5060b8a Compare November 3, 2023 17:59

Feedback - improve tests

ed84fe9

jaykorean force-pushed the periodic_compaction_during_offpeak branch from 5060b8a to ed84fe9 Compare November 3, 2023 18:05

Add release note

1a29f2f

Add a test case

a14eb23

jaykorean force-pushed the periodic_compaction_during_offpeak branch from 2d68d9a to a14eb23 Compare November 3, 2023 21:28

jaykorean force-pushed the periodic_compaction_during_offpeak branch from 7490f2a to 788d092 Compare November 3, 2023 22:29

jaykorean force-pushed the periodic_compaction_during_offpeak branch from 788d092 to 083041e Compare November 3, 2023 22:30

Fix linter

4ccc413

jaykorean force-pushed the periodic_compaction_during_offpeak branch from 083041e to 4ccc413 Compare November 3, 2023 22:32

jaykorean requested review from pdillinger and cbi42 November 3, 2023 23:23

cbi42 approved these changes Nov 6, 2023

View reviewed changes

facebook-github-bot closed this in 2dab137 Nov 6, 2023

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

jaykorean deleted the periodic_compaction_during_offpeak branch November 6, 2023 21:06

igorcanadi mentioned this pull request Jan 17, 2024

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

Closed

Mark more files for periodic compaction during offpeak #12031

Mark more files for periodic compaction during offpeak #12031

Conversation

jaykorean commented Oct 30, 2023 • edited Loading

Summary

Test Plan

facebook-github-bot commented Nov 1, 2023

facebook-github-bot commented Nov 1, 2023

pdillinger 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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaykorean commented Nov 2, 2023

facebook-github-bot commented Nov 3, 2023

facebook-github-bot commented Nov 3, 2023

facebook-github-bot commented Nov 3, 2023

facebook-github-bot commented Nov 3, 2023

facebook-github-bot commented Nov 3, 2023

facebook-github-bot commented Nov 3, 2023

facebook-github-bot commented Nov 3, 2023

facebook-github-bot commented Nov 3, 2023

facebook-github-bot commented Nov 3, 2023

facebook-github-bot commented Nov 3, 2023

facebook-github-bot commented Nov 3, 2023

facebook-github-bot commented Nov 3, 2023

facebook-github-bot commented Nov 3, 2023

facebook-github-bot commented Nov 3, 2023

facebook-github-bot commented Nov 3, 2023

facebook-github-bot commented Nov 3, 2023

cbi42 left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 6, 2023

sakae-nakajima commented Nov 10, 2024

jaykorean commented Nov 15, 2024

adamretter commented Jan 6, 2025

jaykorean commented Jan 6, 2025

jaykorean commented Oct 30, 2023 •

edited

Loading