Add OpenAndTrimHistory API to support trimming data with specified timestamp #9410

sunlike-Lipo · 2022-01-20T11:43:05Z

As disscussed in (#9223), Here added a new API named DB::OpenAndTrimHistory, this API will open DB and trim data to the timestamp specofied by trim_ts (The data with newer timestamp than specified trim bound will be removed). This API should only be used at a timestamp-enabled db instance recovery.

And this PR implemented a new iterator named HistoryTrimmingIterator to support trimming history with a new API named DB::OpenAndTrimHistory. HistoryTrimmingIterator wrapped around the underlying InternalITerator such that keys whose timestamps newer than trim_ts should not be returned to the compaction iterator while trim_ts is not null.

…ange

siying · 2022-01-31T21:21:11Z

Since the previous discussion was with @riversand963 , assigning to @riversand963 to triage.

riversand963 · 2022-02-02T17:21:36Z

Thanks @sunlike-Lipo for the PR and sorry for late reply.
According to #9223 (comment), is this history trimming only needed when we open a database (not sure if manually demotion requires re-open)? If so, I think it's better not to extend this history trimming to CompactRange which can be called at any time, since it's hard to reason about correctness when the db is accepting new writes.
An example is OpenAndCompact(). Similar to this, we can have DB::OpenAndTrimHistory(..., ts).

riversand963

Thanks @sunlike-Lipo for the PR. I left a few comments.

util/history_trimming_iterator.h

db/dbformat.h

util/history_trimming_iterator.h

sunlike-Lipo · 2022-02-07T17:47:33Z

Thanks @sunlike-Lipo for the PR and sorry for late reply. According to #9223 (comment), is this history trimming only needed when we open a database (not sure if manually demotion requires re-open)? If so, I think it's better not to extend this history trimming to CompactRange which can be called at any time, since it's hard to reason about correctness when the db is accepting new writes. An example is OpenAndCompact(). Similar to this, we can have DB::OpenAndTrimHistory(..., ts).

@riversand963
Thank you for your review and very sorry for the late reply...
I added DB::OpenAndTrimHistory() API in the new commit. Please let me know if this API meets your requirements.

sunlike-Lipo · 2022-02-08T04:54:03Z

@riversand963
It seems the review comments have been marked outdated after I resolved merge-conflicts and squash the new commits.
Very sorry for that if it has caused you any inconvenience. I'll be more cautious in dealing with similar situations.

riversand963 · 2022-02-09T17:08:15Z

Thanks @sunlike-Lipo for the PR. I will try to review this week.

riversand963

Thanks @sunlike-Lipo for the improvement.
How do you handle data in the WAL if avoid_flush_during_recovery is true? If possible, these data will remain in memtable.

HISTORY.md

db/compaction/compaction.h

include/rocksdb/db.h

util/history_trimming_iterator.h

db/db_impl/db_impl_open.cc

riversand963 · 2022-02-15T18:12:02Z

How do you handle data in the WAL if avoid_flush_during_recovery is true? If possible, these data will remain in memtable.

Maybe a few options to consider:

Declare that we do not currently support calling the new OpenAndTrim API if avoid_flush_during_recovery is true. Return NotSupported() or InvalidArgument() if user does so.
Implement the trimming in flush code path.
Perform trimming before inserting into memtable during recovery.

I think 1 is good for now, but add TODO for the other two.

sunlike-Lipo · 2022-02-19T02:44:10Z

Thanks @riversand963 review. I will provide a new commits this weekend

* Skip column families that do not enable user-defined timestamp, * Add validateOptions operation in OpenAndTrimHistory, * Modified some comments in the api to clarify that the trim operation of this api is only valid for the column family with timestamp enabled, * Added cleanup operation in case of failure, clean up db and column family

sunlike-Lipo · 2022-02-21T00:45:48Z

How do you handle data in the WAL if avoid_flush_during_recovery is true? If possible, these data will remain in memtable.

Maybe a few options to consider:

Declare that we do not currently support calling the new OpenAndTrim API if avoid_flush_during_recovery is true. Return NotSupported() or InvalidArgument() if user does so.

Implement the trimming in flush code path.

Perform trimming before inserting into memtable during recovery.

I think 1 is good for now, but add TODO for the other two.

@riversand963 1 has been fixed, 2, 3 also added TODO.
btw, I think we should also add BottommostLevelCompaction::kForceOptimized, otherwise, the bottommost-level files may be skipped when CompactRangeInternal.
I also think we can skip files by its min_timestamp and max_timestamp before trimming history, to reduce the total io-cost. Only files with file.max_timestamp > trim_ts are necessary to trim. I will post another PR for optimization after this PR is merged.

…stamp > trim_ts

riversand963

Thanks @sunlike-Lipo for the PR. Left some comments and we are getting close.

include/rocksdb/db.h

db/db_impl/db_impl_open.cc

db/history_trimming_iterator.h

db/db_with_timestamp_basic_test.cc

db/db_impl/db_impl_open.cc

facebook-github-bot · 2022-02-23T03:54:09Z

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

Fix review comments

facebook-github-bot · 2022-02-24T01:54:46Z

@sunlike-Lipo has updated the pull request. You must reimport the pull request before landing.

sunlike-Lipo · 2022-02-24T02:14:25Z

db/history_trimming_iterator.h

@@ -25,7 +27,7 @@ class HistoryTrimmingIterator : public InternalIterator {
      return true;
    }
    Slice current_ts = ExtractTimestampFromKey(key(), cmp_->timestamp_size());
-    return cmp_->CompareTimestamp(current_ts, Slice(filter_ts_)) < 0;
+    return cmp_->CompareTimestamp(current_ts, Slice(filter_ts_)) <= 0;


By the API's definition, data with ts <= filter_ts_ should be reserved, I added a unittest for the = case in the last commit.

riversand963

Thanks @sunlike-Lipo for the PR! LGTM. Can you resolve the conflicts and rebase?

include/rocksdb/db.h

facebook-github-bot · 2022-02-26T02:41:16Z

@sunlike-Lipo has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-02-26T02:41:18Z

@sunlike-Lipo has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-02-26T02:49:49Z

@sunlike-Lipo has updated the pull request. You must reimport the pull request before landing.

sunlike-Lipo · 2022-02-26T03:02:58Z

Thanks @sunlike-Lipo for the PR! LGTM. Can you resolve the conflicts and rebase?

@riversand963
I've fixed the conflicts in HISTORY.md.
Thank you for your review and guidance at last. I've learned a lot from that.

facebook-github-bot · 2022-02-28T16:47:37Z

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

facebook-github-bot · 2022-03-11T21:35:59Z

@sunlike-Lipo has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-03-11T21:37:37Z

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

facebook-github-bot · 2022-03-11T21:58:12Z

@sunlike-Lipo has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-03-11T21:59:30Z

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

Add HistoryTrimmingIterator to support trimming history with CompactR…

427c882

…ange

facebook-github-bot added the CLA Signed label Jan 20, 2022

sunlike-Lipo mentioned this pull request Jan 20, 2022

[Timestamp|Discussion] Weird behavior when Timestamped Del combined with CompactionFilter #9223

Closed

siying requested a review from riversand963 January 31, 2022 21:20

riversand963 reviewed Feb 2, 2022

View reviewed changes

sunlike-Lipo force-pushed the main branch 2 times, most recently from 42de914 to 3b0a0c4 Compare February 7, 2022 17:47

sunlike-Lipo force-pushed the main branch from 3b0a0c4 to 8652d7f Compare February 8, 2022 01:27

fix review comments

a93b097

sunlike-Lipo force-pushed the main branch from 8652d7f to a93b097 Compare February 8, 2022 01:31

sunlike-Lipo added 5 commits February 8, 2022 10:07

update HISTORY.md

e5653ae

revert HISTORY.md and irrelevant blank lines

02df207

Merge branch 'facebook:main' into main

423a8e0

update HISTORY.md

debc9fe

adapt to new Put() and Get() with ts

25f6922

adapt to new Put() with ts

a1dfd95

sunlike-Lipo changed the title ~~Add HistoryTrimmingIterator to support trimming history with CompactRange~~ Add HistoryTrimmingIterator to support trimming history with OpenAndTrimHistory Feb 9, 2022

sunlike-Lipo changed the title ~~Add HistoryTrimmingIterator to support trimming history with OpenAndTrimHistory~~ Add OpenAndTrimHistory API to support trimming data with specified timestamp Feb 9, 2022

riversand963 reviewed Feb 15, 2022

View reviewed changes

HISTORY.md Outdated Show resolved Hide resolved

db/compaction/compaction.h Show resolved Hide resolved

include/rocksdb/db.h Outdated Show resolved Hide resolved

util/history_trimming_iterator.h Outdated Show resolved Hide resolved

riversand963 reviewed Feb 15, 2022

View reviewed changes

db/db_impl/db_impl_open.cc Outdated Show resolved Hide resolved

riversand963 reviewed Feb 15, 2022

View reviewed changes

db/db_impl/db_impl_open.cc Outdated Show resolved Hide resolved

Modify TODO description, Update pick files selection rule to max_time…

d638ac0

…stamp > trim_ts

riversand963 reviewed Feb 23, 2022

View reviewed changes

Fix review comments

e9c7ffa

Fix review comments

sunlike-Lipo commented Feb 24, 2022

View reviewed changes

riversand963 approved these changes Feb 25, 2022

View reviewed changes

include/rocksdb/db.h Outdated Show resolved Hide resolved

include/rocksdb/db.h Outdated Show resolved Hide resolved

sunlike-Lipo added 2 commits February 26, 2022 10:40

fix review comments

7ff47cf

Merge branch 'facebook:main' into main

f4c9b0e

update HISTORY.md

1fa961e

riversand963 added 3 commits March 11, 2022 10:28

Merge remote-tracking branch 'upstream/main' into pr9410

5dcccfe

Fix linter

6ecb6ec

Linter and create trim history iter after blob gc

fa02421

Address comments

a8dd5c3

facebook-github-bot closed this in 95305c4 Mar 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OpenAndTrimHistory API to support trimming data with specified timestamp #9410

Add OpenAndTrimHistory API to support trimming data with specified timestamp #9410

sunlike-Lipo commented Jan 20, 2022 •

edited

Loading

siying commented Jan 31, 2022

riversand963 commented Feb 2, 2022

riversand963 left a comment

sunlike-Lipo commented Feb 7, 2022 •

edited

Loading

sunlike-Lipo commented Feb 8, 2022 •

edited

Loading

riversand963 commented Feb 9, 2022

riversand963 left a comment

riversand963 commented Feb 15, 2022

sunlike-Lipo commented Feb 19, 2022

sunlike-Lipo commented Feb 21, 2022 •

edited

Loading

riversand963 left a comment

facebook-github-bot commented Feb 23, 2022

facebook-github-bot commented Feb 24, 2022

sunlike-Lipo Feb 24, 2022

riversand963 left a comment

facebook-github-bot commented Feb 26, 2022

facebook-github-bot commented Feb 26, 2022

facebook-github-bot commented Feb 26, 2022

sunlike-Lipo commented Feb 26, 2022 •

edited

Loading

facebook-github-bot commented Feb 28, 2022

facebook-github-bot commented Mar 11, 2022

facebook-github-bot commented Mar 11, 2022

facebook-github-bot commented Mar 11, 2022

facebook-github-bot commented Mar 11, 2022

Add OpenAndTrimHistory API to support trimming data with specified timestamp #9410

Add OpenAndTrimHistory API to support trimming data with specified timestamp #9410

Conversation

sunlike-Lipo commented Jan 20, 2022 • edited Loading

siying commented Jan 31, 2022

riversand963 commented Feb 2, 2022

riversand963 left a comment

Choose a reason for hiding this comment

sunlike-Lipo commented Feb 7, 2022 • edited Loading

sunlike-Lipo commented Feb 8, 2022 • edited Loading

riversand963 commented Feb 9, 2022

riversand963 left a comment

Choose a reason for hiding this comment

riversand963 commented Feb 15, 2022

sunlike-Lipo commented Feb 19, 2022

sunlike-Lipo commented Feb 21, 2022 • edited Loading

riversand963 left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 23, 2022

facebook-github-bot commented Feb 24, 2022

sunlike-Lipo Feb 24, 2022

Choose a reason for hiding this comment

riversand963 left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Feb 26, 2022

facebook-github-bot commented Feb 26, 2022

facebook-github-bot commented Feb 26, 2022

sunlike-Lipo commented Feb 26, 2022 • edited Loading

facebook-github-bot commented Feb 28, 2022

facebook-github-bot commented Mar 11, 2022

facebook-github-bot commented Mar 11, 2022

facebook-github-bot commented Mar 11, 2022

facebook-github-bot commented Mar 11, 2022

sunlike-Lipo commented Jan 20, 2022 •

edited

Loading

sunlike-Lipo commented Feb 7, 2022 •

edited

Loading

sunlike-Lipo commented Feb 8, 2022 •

edited

Loading

sunlike-Lipo commented Feb 21, 2022 •

edited

Loading

sunlike-Lipo commented Feb 26, 2022 •

edited

Loading