Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add timestamp support to DBImplReadOnly #10004

Closed
wants to merge 3 commits into from
Closed

Add timestamp support to DBImplReadOnly #10004

wants to merge 3 commits into from

Conversation

jowlyzhang
Copy link
Contributor

This PR adds timestamp support to a read only DB instance opened as DBImplReadOnly. A follow up PR will add the same support to CompactedDBImpl.

With this, read only database has these timestamp related APIs:

ReadOptions.timestamp : read should return the latest data visible to this specified timestamp
Iterator::timestamp() : returns the timestamp associated with the key, value
DB:Get(..., std::string* timestamp) : returns the timestamp associated with the key, value in timestamp

Test plan (on devserver):

$COMPILE_WITH_ASAN=1 make -j24 all
$./db_with_timestamp_basic_test --gtest_filter=DBBasicTestWithTimestamp.ReadOnlyDB*

@facebook-github-bot
Copy link
Contributor

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

1 similar comment
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

riversand963
riversand963 reviewed May 17, 2022
View reviewed changes
Copy link
Contributor

@riversand963 riversand963 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jowlyzhang for the PR. Mostly LGTM with a few minor comments.

Can we add some test coverage for the error cases for Get(), NewIterator() and NewIterators()?
Furthermore, can you update HISTORY.md, or should we update it after updating CompactedDBImpl (in another PR)?

db/db_with_timestamp_basic_test.cc Outdated Show resolved Hide resolved
db/db_impl/db_impl_readonly.h Outdated Show resolved Hide resolved
db/db_impl/db_impl_readonly.cc Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@jowlyzhang
Copy link
Contributor Author

@jowlyzhang jowlyzhang closed this May 18, 2022
@jowlyzhang jowlyzhang reopened this May 18, 2022
@jowlyzhang
Copy link
Contributor Author

unmerged

Thanks @jowlyzhang for the PR. Mostly LGTM with a few minor comments.

Can we add some test coverage for the error cases for Get(), NewIterator() and NewIterators()? Furthermore, can you update HISTORY.md, or should we update it after updating CompactedDBImpl (in another PR)?

Thank you for the suggestion. I will update the HISTORY.md file in the follow up PR after CompactedDBImpl supports timestamp too.

@facebook-github-bot
Copy link
Contributor

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

riversand963
riversand963 reviewed May 18, 2022
View reviewed changes
Copy link
Contributor

@riversand963 riversand963 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test failure is legit and can be fixed by checking the return status.
Otherwise LGTM.

db/db_readonly_with_timestamp_test.cc Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

riversand963
riversand963 approved these changes May 18, 2022
View reviewed changes
Copy link
Contributor

@riversand963 riversand963 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jowlyzhang for the PR!

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@jowlyzhang jowlyzhang deleted the read_only_impl_ts branch May 20, 2022 16:22
@riversand963 riversand963 mentioned this pull request May 24, 2022
Closed
facebook-github-bot pushed a commit that referenced this pull request May 25, 2022
@riversand963 @facebook-github-bot
Enable checkpoint and backup in db_stress when timestamp is enabled (#…
9901e7f 
…10047)

Summary:
After #10030 and #10004, we can enable checkpoint and backup in stress tests when
user-defined timestamp is enabled.

This PR has no production risk.

Pull Request resolved: #10047

Test Plan:
```
TEST_TMPDIR=/dev/shm make crash_test_with_ts
```

Reviewed By: jowlyzhang

Differential Revision: D36641565

Pulled By: riversand963

fbshipit-source-id: d86c9d87efcc34c32d1aa176af691d32b897644a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@riversand963 riversand963 riversand963 approved these changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jowlyzhang @facebook-github-bot @riversand963