Track per-SST user-defined timestamp information in MANIFEST #9092

sunlike-Lipo · 2021-10-29T10:18:48Z

Track per-SST user-defined timestamp information in MANIFEST #8957

Rockdb has supported user-defined timestamp feature. Application can specify a timestamp
when writing each k-v pair. When data flush from memory to disk file called SST files, file
creation activity will commit to MANIFEST. This commit is for tracking timestamp info in the
MANIFEST for each file. The changes involved are as follows:

Summary:

Track max/min timestamp in FileMetaData, and fix invoved codes.
Add NewFileCustomTag::kMinTimestamp and NewFileCustomTag::kMinTimestamp in
NewFileCustomTag ( in the kNewFile4 part ), and support invoved codes such as
VersionEdit Encode and Decode etc.
Add unit test code for VersionEdit EncodeDecodeNewFile4, and fix invoved test codes.

wolfkdy · 2021-11-04T01:44:14Z

@riversand963 I've reviewed this mr and my colleague post it.
The original mr is splitted into two mrs.
What you commented in the original mr are all fixed.
Shall you take a look when free, thanks!

riversand963

Sorry again for the delay, have been a little busier lately.
Thanks @sunlike-Lipo for the PR, what this PR currently has LGTM.
I cannot find the logic of actually computing min/max timestamp for each file during flush/compaction/ingestion/..., is that going to me in another PR, or am I missing anything here?
cc @wolfkdy

riversand963 · 2021-11-04T03:12:08Z

db/version_edit.cc

@@ -186,6 +186,13 @@ bool VersionEdit::EncodeTo(std::string* dst) const {
    PutVarint32(dst, NewFileCustomTag::kFileChecksumFuncName);
    PutLengthPrefixedSlice(dst, Slice(f.file_checksum_func_name));

+    if (f.max_timestamp != kDisableUserTimestamp) {
+      assert(f.min_timestamp != kDisableUserTimestamp);


Can we return false if assertion fails?

sure, fixed

riversand963 · 2021-11-04T03:14:02Z

db/version_edit.cc

+          f.min_timestamp = field.ToString();
+          break;
+        case kMaxTimestamp:
+          f.max_timestamp = field.ToString();


Can we add a check for the equality of min_timestamp.size() and max_timestamp.size() somewhere in this method?

sure, added

riversand963 · 2021-11-04T03:25:43Z

oh, you split that to #9093 .
I can see the confusion now. What I originally meant was to also store the min/max timestamp in the properties block of each SST, which can be in another PR, (I think).
What #9093 does is to compute min/max timestamps using properties collectors.
nvm, we can still merge them separately.

…8957

riversand963 · 2021-11-05T21:24:41Z

I also suggest that the description of this PR be updated since it's too short now. It would be great if the description can briefly mention we introduce (compatible) format changes to MANIFEST, etc.

riversand963

Left a few more comments.

riversand963 · 2021-11-05T21:27:29Z

db/version_edit.cc

@@ -186,6 +186,15 @@ bool VersionEdit::EncodeTo(std::string* dst) const {
    PutVarint32(dst, NewFileCustomTag::kFileChecksumFuncName);
    PutLengthPrefixedSlice(dst, Slice(f.file_checksum_func_name));

+    if (f.max_timestamp != kDisableUserTimestamp) {
+      if (f.min_timestamp == kDisableUserTimestamp) {


I prefer a more strict check.

if (f.min_timestamp.size() != f.max_timestamp.size()) { assert(false); return false; }

ok, I fixed it as your suggestion.

riversand963 · 2021-11-05T21:31:36Z

db/version_edit.cc

@@ -315,6 +324,7 @@ const char* VersionEdit::DecodeNewFile4From(Slice* input) {
        return "new-file4 custom field";
      }
      if (custom_tag == kTerminate) {
+        assert(f.min_timestamp.size() == f.max_timestamp.size());


In case of error, we need to return a string literal to indicate error. THe assertion will be optimized away in opt build mode.

ok, I added some return msg after assert.

…8957 Rockdb has supported user-defined timestamp feature. Application can specify a timestamp when writing each k-v pair. When data flush from memory to disk file called SST files, file creation activity will commit to MANIFEST. This commit is for tracking timestamp info in the MANIFEST for each file. The changes involved are as follows: 1) Track max/min timestamp in FileMetaData, and fix invoved codes. 2) Add max/min timestamp in NewFileCustomTag(in the knEWfILE4 part), and support invoved codes such as VersionEdit Encode and Decode etc. 3) Add unit test code for VersionEdit EncodeDecodeNewFile4, and fix invoved test codes.

sunlike-Lipo · 2021-11-08T02:59:04Z

I also suggest that the description of this PR be updated since it's too short now. It would be great if the description can briefly mention we introduce (compatible) format changes to MANIFEST, etc.

I add some description about this pr. Thanks a lot for suggestion.

riversand963

Thanks @sunlike-Lipo for the PR. Mostly LGTM except two minor comments.

riversand963 · 2021-11-08T17:47:06Z

db/version_edit.cc

+    r.append(" min_timestamp:");
+    r.append(f.min_timestamp);
+    r.append(" max_timestamp:");
+    r.append(f.max_timestamp);


Sorry for this late comment.
I think printing the timestamps in hex format seems better.

OK. now fix it to Slice(f.min_timestamp).ToString(true).

riversand963 · 2021-11-08T17:47:34Z

db/version_edit.cc

+        jw << "MinTimestamp" << f.min_timestamp;
+        jw << "MaxTimestamp" << f.max_timestamp;


Print timestamp in hex format?

OK. now fix it to Slice(f.min_timestamp).ToString(true).

facebook-github-bot · 2021-11-08T17:52:43Z

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

facebook-github-bot · 2021-11-09T01:50:07Z

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

facebook-github-bot · 2021-11-09T04:53:56Z

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

riversand963 · 2021-11-09T05:14:41Z

Turns out our internal linter is unhappy about putting kDisableUserTimestamp definition in dbformat.h. Can we declare it in dbformat.h and define it in dbformat.cc?

facebook-github-bot · 2021-11-09T06:37:12Z

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

sunlike-Lipo · 2021-11-09T07:35:17Z

Turns out our internal linter is unhappy about putting kDisableUserTimestamp definition in dbformat.h. Can we declare it in dbformat.h and define it in dbformat.cc?

Sure. That sounds more reasonable. fixed

facebook-github-bot · 2021-11-09T17:22:09Z

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

riversand963 · 2021-11-10T00:25:22Z

Thanks @sunlike-Lipo a lot for the PR! I have already initiated internal review process for merging.
Just realized one minor thing, maybe we should still mention this PR in HISTORY.md although this PR does not currently enable the functionality of tracking per-file timestamps. I was a little hesitant before, but now I think we should still update HISTORY. After that, I think we are good to go.

facebook-github-bot · 2021-11-10T02:01:50Z

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

facebook-github-bot · 2021-11-10T02:06:37Z

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

facebook-github-bot · 2021-11-10T02:11:37Z

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

sunlike-Lipo · 2021-11-10T02:56:58Z

Thanks @sunlike-Lipo a lot for the PR! I have already initiated internal review process for merging. Just realized one minor thing, maybe we should still mention this PR in HISTORY.md although this PR does not currently enable the functionality of tracking per-file timestamps. I was a little hesitant before, but now I think we should still update HISTORY. After that, I think we are good to go.

OK, I modified the HISTORY.md.

facebook-github-bot · 2021-11-10T18:49:18Z

@riversand963 merged this pull request in 937fbcb.

facebook-github-bot added the CLA Signed label Oct 29, 2021

sunlike-Lipo force-pushed the main branch from 4b3a1ec to 1aaaa43 Compare October 29, 2021 11:58

sunlike-Lipo changed the title ~~add some description about timestamp in metadata~~ Track per-SST user-defined timestamp information in MANIFEST Oct 29, 2021

sunlike-Lipo mentioned this pull request Nov 4, 2021

Track per-SST user-defined timestamp information in MANIFEST #8957

Closed

riversand963 reviewed Nov 4, 2021

View reviewed changes

wolfkdy mentioned this pull request Nov 4, 2021

Track each SST's timestamp information as user properties #9093

Closed

sunlike-Lipo force-pushed the main branch from 1aaaa43 to ff16ba1 Compare November 4, 2021 09:42

Track per-SST user-defined timestamp infomation in MANIFEST facebook#…

aa34ac8

…8957

sunlike-Lipo force-pushed the main branch from ff16ba1 to aa34ac8 Compare November 4, 2021 10:15

wolfkdy mentioned this pull request Nov 5, 2021

support rocksdb 6.x series mongodb-partners/mongo-rocks#164

Open

riversand963 reviewed Nov 5, 2021

View reviewed changes

sunlike-Lipo force-pushed the main branch from 8af5c8f to ea4e511 Compare November 8, 2021 02:29

sunlike-Lipo force-pushed the main branch from ea4e511 to 8422112 Compare November 8, 2021 02:50

Merge branch 'main' into main

f7616c6

riversand963 approved these changes Nov 8, 2021

View reviewed changes

fix review comments

14fe332

fix review comments

d8ba594

update HISTORY.md about this mr

34fbe65

sunlike-Lipo force-pushed the main branch from 8f52f59 to 34fbe65 Compare November 10, 2021 02:06

facebook-github-bot closed this in 937fbcb Nov 10, 2021

facebook-github-bot added the Merged label Nov 10, 2021

riversand963 mentioned this pull request Aug 1, 2022

Remove unused fields from FileMetaData (temporarily) #10443

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track per-SST user-defined timestamp information in MANIFEST #9092

Track per-SST user-defined timestamp information in MANIFEST #9092

sunlike-Lipo commented Oct 29, 2021 •

edited

Loading

wolfkdy commented Nov 4, 2021

riversand963 left a comment

riversand963 Nov 4, 2021

sunlike-Lipo Nov 4, 2021

riversand963 Nov 4, 2021

sunlike-Lipo Nov 4, 2021

riversand963 commented Nov 4, 2021

riversand963 commented Nov 5, 2021

riversand963 left a comment

riversand963 Nov 5, 2021

sunlike-Lipo Nov 8, 2021

riversand963 Nov 5, 2021

sunlike-Lipo Nov 8, 2021

sunlike-Lipo commented Nov 8, 2021

riversand963 left a comment

riversand963 Nov 8, 2021

sunlike-Lipo Nov 9, 2021

riversand963 Nov 8, 2021

sunlike-Lipo Nov 9, 2021

facebook-github-bot commented Nov 8, 2021

facebook-github-bot commented Nov 9, 2021

facebook-github-bot commented Nov 9, 2021

riversand963 commented Nov 9, 2021

facebook-github-bot commented Nov 9, 2021

sunlike-Lipo commented Nov 9, 2021

facebook-github-bot commented Nov 9, 2021

riversand963 commented Nov 10, 2021

facebook-github-bot commented Nov 10, 2021

facebook-github-bot commented Nov 10, 2021

facebook-github-bot commented Nov 10, 2021

sunlike-Lipo commented Nov 10, 2021

facebook-github-bot commented Nov 10, 2021

		jw << "MinTimestamp" << f.min_timestamp;
		jw << "MaxTimestamp" << f.max_timestamp;

Track per-SST user-defined timestamp information in MANIFEST #9092

Track per-SST user-defined timestamp information in MANIFEST #9092

Conversation

sunlike-Lipo commented Oct 29, 2021 • edited Loading

wolfkdy commented Nov 4, 2021

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

Choose a reason for hiding this comment

riversand963 commented Nov 4, 2021

riversand963 commented Nov 5, 2021

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

Choose a reason for hiding this comment

sunlike-Lipo commented Nov 8, 2021

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

Choose a reason for hiding this comment

facebook-github-bot commented Nov 8, 2021

facebook-github-bot commented Nov 9, 2021

facebook-github-bot commented Nov 9, 2021

riversand963 commented Nov 9, 2021

facebook-github-bot commented Nov 9, 2021

sunlike-Lipo commented Nov 9, 2021

facebook-github-bot commented Nov 9, 2021

riversand963 commented Nov 10, 2021

facebook-github-bot commented Nov 10, 2021

facebook-github-bot commented Nov 10, 2021

facebook-github-bot commented Nov 10, 2021

sunlike-Lipo commented Nov 10, 2021

facebook-github-bot commented Nov 10, 2021

sunlike-Lipo commented Oct 29, 2021 •

edited

Loading