Add option `preserve_internal_time_seconds` to preserve the time info #10747

jay-zhuang · 2022-09-27T18:48:09Z

Add option preserve_internal_time_seconds to preserve the internal
time information.
It's mostly for the migration of the existing data to tiered storage (
preclude_last_level_data_seconds). When the tiering feature is just
enabled, the existing data won't have the time information to decide if
it's hot or cold. Enabling this feature will start collect and preserve
the time information for the new data.

facebook-github-bot · 2022-09-28T23:08:29Z

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

siying · 2022-10-04T20:26:26Z

HISTORY.md

@@ -6,6 +6,9 @@
 * Fixed an optimistic transaction validation bug caused by DBImpl::GetLatestSequenceForKey() returning non-latest seq for merge (#10724).
 * Fixed a bug in iterator refresh which could segfault for DeleteRange users (#10739).

+### New Features
+* Add option `preserve_internal_time_seconds` for tiered storage migration, which can start collecting the preserving the time information for the new data, but won't enable the tiering feature (`preclude_last_level_data_seconds`) yet. The time information can be used to decide the temperature of the existing data when the tiering is enabled later.


preserve_internal_time_seconds can potentially be used for more accurate oldest ancestor time estimation, right? So maybe explain it more in a boarder way.

The feature hasn't been implemented yet (we abandoned #8185 because based on test, there's limited improvement with random dataset).

I know. I mean just be vague about what the feature does, and not limit to tiered storage. Even now, the feature can be used for manual checking the age of the keys.

siying · 2022-10-04T20:27:46Z

db/compaction/compaction_job.cc

+  uint64_t preserve_time_duration =
+      c->immutable_options()->preclude_last_level_data_seconds == 0
+          ? c->immutable_options()->preserve_internal_time_seconds
+          : c->immutable_options()->preclude_last_level_data_seconds;


Should it be max(preserve_internal_time_seconds, preclude_last_level_data_seconds)?

Yeah, I was thinking about that too, but only slightly leaning to have "preclude_last_level" to override "preserve_time":

For simplification, so only one number is valid.

Implementation wise, it also simpler, that only one number needs to be stored in compaction_job.

But the main question is: is there usecase that the use want to set these 2 number differently?
Currently I think the feature is still be part of "preclude_last_level".

Enable time tracking may improve the oldest_ancestor_time estimation, but the feature hasn't be implemented and for normal dataset, it doesn't improve the estimation too much based on some simulator test (that's one of the reason we abandon the original PR #8185 and currently we hope RoundRobin can fundamentally fix the TTL compaction issue).

Even if we only limit the feature to tiered storage (which I would argue we shouldn't), I assume we would leave room where preserve_internal_time_seconds > preclude_last_level_data_seconds, if users would preserve the choice that they would increase preclude_last_level_data_seconds later and hope the it takes effective sooner.

siying · 2022-10-04T20:28:36Z

db/db_impl/db_impl.cc

+      uint64_t track_time_duration =
+          cfd->ioptions()->preclude_last_level_data_seconds == 0
+              ? cfd->ioptions()->preserve_internal_time_seconds
+              : cfd->ioptions()->preclude_last_level_data_seconds;


Same, should it be max() if preclude_last_level_data_seconds <> 0?

siying · 2022-10-04T20:29:38Z

include/rocksdb/advanced_options.h

+  //  It's typically used for the migration of existing data to tiered storage.
+  //  The user can enable this feature first, then the existing data will have
+  //  time information for `preclude_last_level_data_seconds` feature which will
+  //  be enabled later.


Comments usually explain the default value and its behavior. We should say whether it is dynamically changeable or not. I think it should be changeable if possible.

I'll add comment to specify that it's not dynamic changeable for now (the same as "preclude_last_level"). And give more information about what it's doing.

facebook-github-bot · 2022-10-06T03:16:31Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

siying · 2022-10-06T18:07:32Z

db/seqno_time_test.cc

+  SeqnoTimeTablePropTest() : SeqnoTimeTest() {}
+
+  void SetTrackTimeDurationOptions(Options& options,
+                                   uint64_t track_time_duration) const {


Nit: Google C++ Style suggests "put all input-only parameters before any output parameters".

siying · 2022-10-06T18:10:46Z

db/seqno_time_test.cc

+
+  ASSERT_OK(db_->CompactRange(cro, nullptr, nullptr));
+
+  ASSERT_GT(num_seqno_zeroing, 0);


It will be nice to cover the case where in the last level, some keys are zeroed out and some aren't, based on time of insertion.

It is also better to validate by reading internal keys too. Perhaps GetAllKeyVersions() can be used for the validation.

siying · 2022-10-06T18:33:04Z

db/compaction/compaction_job.cc

+              ? _current_time - preserve_time_duration
+              : 0;
+      preserve_time_min_seqno_ =
+          seqno_time_mapping_.GetOldestSequenceNum(preserve_time);


I didn't completely get why now the oldest entry isn't seqno to preserve anymore, but at least it's still correct.

Yeah, you're right, it is the oldest entry for this version of code. (my first version was having preserve_time and preclude_time differently, then I changed mind to have preclude_time to override preserve_time if both set, but forgot to revert this code. Now I think we're going back to have them differently).

…king

facebook-github-bot · 2022-10-07T21:34:31Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-10-07T22:00:50Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-10-07T22:25:05Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-10-07T22:35:15Z

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

facebook-github-bot · 2022-10-07T23:16:54Z

@jay-zhuang has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-10-07T23:23:07Z

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

Lock the penultimate level for the whole compaction inputs range, so any key in that compaction is safe to move up from the last level to penultimate level. It depends on facebook#10747 (for the migration test)

facebook-github-bot added the CLA Signed label Sep 27, 2022

jay-zhuang force-pushed the timetracking branch 2 times, most recently from 47e0f50 to 4fceba5 Compare September 28, 2022 22:25

jay-zhuang changed the title ~~Add option track_internal_time_seconds to enable internal time tracking~~ Add option preserve_internal_time_seconds to preserve the time info Sep 28, 2022

jay-zhuang marked this pull request as ready for review September 28, 2022 23:08

jay-zhuang requested a review from siying October 4, 2022 18:26

siying reviewed Oct 4, 2022

View reviewed changes

jay-zhuang force-pushed the timetracking branch from 1beaeb2 to 3f822df Compare October 6, 2022 03:16

jay-zhuang mentioned this pull request Oct 6, 2022

Allow the last level data moving up to penultimate level #10782

Closed

siying approved these changes Oct 6, 2022

View reviewed changes

jay-zhuang added 10 commits October 7, 2022 14:34

Add option track_internal_time_seconds to enable internal time trac…

d117e15

…king

Update test

63ba95b

Add new option

0ca0dce

refactor the name

1d0ee6c

fix build

bde8d7c

Update doc

7665a2f

Update comments

8339507

split preserve_time and preclude_last_level option

085012d

Fix a bug

50daf79

Add test

d56a5f5

jay-zhuang force-pushed the timetracking branch from 3f822df to d56a5f5 Compare October 7, 2022 21:34

jay-zhuang added 2 commits October 7, 2022 14:49

Fix build

6c5ae82

Update comments

4e657ce

Add HISTORY

cf89629

remove debug code

effa0b0

facebook-github-bot closed this in c401f28 Oct 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option `preserve_internal_time_seconds` to preserve the time info #10747

Add option `preserve_internal_time_seconds` to preserve the time info #10747

jay-zhuang commented Sep 27, 2022 •

edited

Loading

facebook-github-bot commented Sep 28, 2022

siying Oct 4, 2022

jay-zhuang Oct 5, 2022

siying Oct 6, 2022

siying Oct 4, 2022

jay-zhuang Oct 5, 2022

siying Oct 6, 2022

siying Oct 4, 2022

siying Oct 4, 2022

jay-zhuang Oct 5, 2022 •

edited

Loading

facebook-github-bot commented Oct 6, 2022

siying Oct 6, 2022

siying Oct 6, 2022

siying Oct 6, 2022

jay-zhuang Oct 7, 2022

facebook-github-bot commented Oct 7, 2022

facebook-github-bot commented Oct 7, 2022

facebook-github-bot commented Oct 7, 2022

facebook-github-bot commented Oct 7, 2022

facebook-github-bot commented Oct 7, 2022

facebook-github-bot commented Oct 7, 2022


		ASSERT_OK(db_->CompactRange(cro, nullptr, nullptr));

		ASSERT_GT(num_seqno_zeroing, 0);

Add option preserve_internal_time_seconds to preserve the time info #10747

Add option preserve_internal_time_seconds to preserve the time info #10747

Conversation

jay-zhuang commented Sep 27, 2022 • edited Loading

facebook-github-bot commented Sep 28, 2022

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

Choose a reason for hiding this comment

jay-zhuang Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Oct 6, 2022

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 7, 2022

facebook-github-bot commented Oct 7, 2022

facebook-github-bot commented Oct 7, 2022

facebook-github-bot commented Oct 7, 2022

facebook-github-bot commented Oct 7, 2022

facebook-github-bot commented Oct 7, 2022

Add option `preserve_internal_time_seconds` to preserve the time info #10747

Add option `preserve_internal_time_seconds` to preserve the time info #10747

jay-zhuang commented Sep 27, 2022 •

edited

Loading

jay-zhuang Oct 5, 2022 •

edited

Loading