tools: use provided options instead of the default #4839
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The current implementation hardcode the default options in different places, which makes it impossible to support other environments (like encrypted environment).
|
Great! Thank you for your contribution. Can you add coverage in unit tests? |
|
Added some tests, PTAL. |
sagar0
approved these changes
Jan 3, 2019
There was a problem hiding this comment.
@sagar0 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
DorianZheng
pushed a commit
to tikv/rocksdb
that referenced
this pull request
Jan 4, 2019
* tools: use provided options instead of the default (facebook#4839) Summary: The current implementation hardcode the default options in different places, which makes it impossible to support other environments (like encrypted environment). Pull Request resolved: facebook#4839 Differential Revision: D13573578 Pulled By: sagar0 fbshipit-source-id: 76b58b4b758902798d10ff2f52d9f39abff015e7 * Fix skip WAL for whole write_group when leader's callback fail (facebook#4838) Summary: The original implementation has two problems: 1. https://github.com/facebook/rocksdb/blob/f0dda35d7de1fd56e0b7c96376ca8aff2a6364fd/db/db_impl_write.cc#L478 https://github.com/facebook/rocksdb/blob/f0dda35d7de1fd56e0b7c96376ca8aff2a6364fd/db/write_thread.h#L231 If the callback status of leader of the write_group fails, then the whole write_group will not write to WAL, this may cause data loss. 2. https://github.com/facebook/rocksdb/blob/f0dda35d7de1fd56e0b7c96376ca8aff2a6364fd/db/write_thread.h#L130 The annotation says that Writer.status is the status of memtable inserter, but the original implementation use it for another case which is not consistent with the original design. Looks like we can still reuse Writer.status, but we should modify the annotation, so Writer.status is not only the status of memtable inserter. Pull Request resolved: facebook#4838 Differential Revision: D13574070 Pulled By: yiwu-arbug fbshipit-source-id: a2a2aefcfd329c4c6a91652bf090aaf1ce119c4b
yiwu-arbug
pushed a commit
to tikv/rocksdb
that referenced
this pull request
Mar 20, 2019
Summary: The current implementation hardcode the default options in different places, which makes it impossible to support other environments (like encrypted environment). Pull Request resolved: facebook#4839 Differential Revision: D13573578 Pulled By: sagar0 fbshipit-source-id: 76b58b4b758902798d10ff2f52d9f39abff015e7
yiwu-arbug
pushed a commit
to tikv/rocksdb
that referenced
this pull request
Mar 20, 2019
Summary: The current implementation hardcode the default options in different places, which makes it impossible to support other environments (like encrypted environment). Pull Request resolved: facebook#4839 Differential Revision: D13573578 Pulled By: sagar0 fbshipit-source-id: 76b58b4b758902798d10ff2f52d9f39abff015e7
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current implementation hardcode the default options in different
places, which makes it impossible to support other environments (like
encrypted environment).