Add a ConfigOptions for use in comparing objects and converting to/from strings #6389
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
mrambacher
force-pushed
the
ConfigOptions
branch
from
0b95020
to
b75c555
Compare
mrambacher
force-pushed
the
ConfigOptions
branch
2 times, most recently
from
f531ff7
to
622945a
Compare
mrambacher
force-pushed
the
ConfigOptions
branch
5 times, most recently
from
ffd277f
to
579e5d0
Compare
mrambacher
force-pushed
the
ConfigOptions
branch
3 times, most recently
from
095ef14
to
f102429
Compare
zhichao-cao
reviewed
Apr 8, 2020
zhichao-cao
reviewed
Apr 8, 2020
zhichao-cao
reviewed
Apr 8, 2020
zhichao-cao
reviewed
Apr 8, 2020
zhichao-cao
reviewed
Apr 8, 2020
|
@adamretter Hi Adam, can you help to review the Java code change of this PR. I'm not familiar with the Java interfaces in RocksDB |
zhichao-cao
reviewed
Apr 14, 2020
zhichao-cao
reviewed
Apr 14, 2020
zhichao-cao
reviewed
Apr 14, 2020
zhichao-cao
reviewed
Apr 14, 2020
|
Also, for all the new interfaces, we want to make them align with our codebase: move ConfigOption to the first place of the function parameters if the functions use ConfigOption as an operating option. |
|
Thanks @mrambacher for working on this! It makes compare/convert objects to/from strings more clear by introducing the ConfigOption, and avoids the fragmented control parameters used in different places. |
mrambacher
force-pushed
the
ConfigOptions
branch
from
f102429
to
54b7433
Compare
|
I believe everything has been resolved (code and comment-wise) from this PR. The only outstanding issue is to add something to HISTORY.MD |
adamretter
requested changes
Apr 19, 2020
mrambacher
force-pushed
the
ConfigOptions
branch
from
cd1be7c
to
0e0f9fb
Compare
mrambacher
force-pushed
the
ConfigOptions
branch
from
0e0f9fb
to
c52f7a2
Compare
adamretter
reviewed
Apr 21, 2020
java/rocksjni/config_options.cc
Outdated
| #include "rocksdb/convenience.h" | ||
|
|
||
| // The portal class for org.rocksdb.SanityLevel | ||
| class SanityLevelJni { |
There was a problem hiding this comment.
Can you move SanityLevelJni to portal.h please
adamretter
reviewed
Apr 21, 2020
java/rocksjni/config_options.cc
Outdated
| } | ||
| } | ||
|
|
||
| // Returns the equivalent C++ ROCKSDB_NAMESPACE::CompactionStopStyle enum for |
adamretter
requested changes
Apr 21, 2020
mrambacher
force-pushed
the
ConfigOptions
branch
from
c52f7a2
to
9c45905
Compare
|
@adamretter Thanks a lot for your detailed review and comments! |
The ConfigOptions encapsulates the additional parameters to the GetFromString, GetFromMap, GetStringFrom, and Verify methods. The GetFromMap typically took two parameters (input_strings_escaped, ignore_unknown_options). The GetStringFrom method would take a parameter for the delimiter between fields. The Verify would take a SanityCheckLevel to control how close to match properties. All of these fields were moved under a single struct. The purpose of this change is to be able to add additional optional parameters to these methods without changing the method signatures in the future.
mrambacher
force-pushed
the
ConfigOptions
branch
from
9c45905
to
45584cc
Compare
There was a problem hiding this comment.
@zhichao-cao has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@zhichao-cao merged this pull request in 4cbc19d. |
facebook-github-bot
pushed a commit
that referenced
this pull request
May 7, 2020
Summary: #6389 replaced the #include of options.h in table.h with forward declarations, which is causing some build failures in RocksDB users in 6.10. Remove the forward declarations and #include options.h as recommended by the style guide - https://google.github.io/styleguide/cppguide.html#Forward_Declarations Pull Request resolved: #6823 Reviewed By: riversand963 Differential Revision: D21464078 Pulled By: anand1976 fbshipit-source-id: 6033ee2544d279690f57bb0db91bc83816cee11d
anand1976
pushed a commit
that referenced
this pull request
May 8, 2020
Summary: #6389 replaced the #include of options.h in table.h with forward declarations, which is causing some build failures in RocksDB users in 6.10. Remove the forward declarations and #include options.h as recommended by the style guide - https://google.github.io/styleguide/cppguide.html#Forward_Declarations Pull Request resolved: #6823 Reviewed By: riversand963 Differential Revision: D21464078 Pulled By: anand1976 fbshipit-source-id: 6033ee2544d279690f57bb0db91bc83816cee11d
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 methods in convenience.h are used to compare/convert objects to/from strings. There is a mishmash of parameters in use here with more needed in the future. This PR replaces those parameters with a single structure.