-
Notifications
You must be signed in to change notification settings - Fork 6.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add commit marker with timestamp #9266
Conversation
This pull request was exported from Phabricator. Differential Revision: D31721350 |
This pull request was exported from Phabricator. Differential Revision: D31721350 |
7e3be40
to
b7d5410
Compare
This pull request was exported from Phabricator. Differential Revision: D31721350 |
b7d5410
to
53113c2
Compare
This pull request was exported from Phabricator. Differential Revision: D31721350 |
53113c2
to
727fb4c
Compare
This pull request was exported from Phabricator. Differential Revision: D31721350 |
727fb4c
to
941622a
Compare
This pull request was exported from Phabricator. Differential Revision: D31721350 |
941622a
to
33d5495
Compare
33d5495
to
9f7ec0c
Compare
This pull request was exported from Phabricator. Differential Revision: D31721350 |
1 similar comment
This pull request was exported from Phabricator. Differential Revision: D31721350 |
9f7ec0c
to
7be487b
Compare
@@ -2114,6 +2149,76 @@ class MemTableInserter : public WriteBatch::Handler { | |||
return s; | |||
} | |||
|
|||
Status MarkCommitWithTimestamp(const Slice& name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice to reuse/share some code between MarkCommit
and MarkCommitWithTimestamp
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking of keeping this mid-sized PR simple such that it does not change existing logic used intensively by production workloads at this sensitive time of year :).
I am also not very sure if future changes to timestamp/transaction will make these two methods' implementation diverge for more complex transaction write-policies, e.g. write-prepared/write-unprepared. Maybe it's not the worst idea just to leave them separate for now.
Thanks for the PR @riversand963 ! LGTM in general, just one comment. P.S. Also, there are a couple of Java failures that seem legit |
Summary: Pull Request resolved: facebook#9266 This diff adds a new tag `CommitWithTimestamp`. Currently, there is no API to trigger writing this tag to WAL, thus it is unavailable to users. This is an ongoing effort to add user-defined timestamp support to write-committed transactions. This diff also indicates all column families that may potentially participate in the same transaction must either disable timestamp or have the same timestamp format, since `CommitWithTimestamp` tag is followed by a single byte-array denoting the commit timestamp of the transaction. We will enforce this checking in a future diff. We keep this diff small. Reviewed By: ltamasi Differential Revision: D31721350 fbshipit-source-id: 16636d7441d79eec3cda73bff4b699f545e7aeab
This pull request was exported from Phabricator. Differential Revision: D31721350 |
7be487b
to
f087f42
Compare
Thanks @ltamasi for the review! |
Summary:
This diff adds a new tag
CommitWithTimestamp
. Currently, there is no API to trigger writingthis tag to WAL, thus it is unavailable to users.
This is an ongoing effort to add user-defined timestamp support to write-committed transactions.
This diff also indicates all column families that may potentially participate in the same
transaction must either disable timestamp or have the same timestamp format, since
CommitWithTimestamp
tag is followed by a single byte-array denoting the committimestamp of the transaction. We will enforce this checking in a future diff. We keep this
diff small.
Differential Revision: D31721350