Bring the Configurable options together #5753
Conversation
|
Hi @mrambacher , starting to look at the configurable PRs. I noticed that #5753 seems to be a combination of #5362, #5497, #5752 and https://github.com/facebook/rocksdb/pull/5574/files |
|
#5574 is the base PR, which adds more functionality to the OptionTypeInfo class. #5361 (Configurable class) uses #5574 to implement a base configurable class. #5362, #5497, and #5752 use the Configurable class to configure their respective pieces whereas #5753 merges those respective pieces into one chunk. I would recommend reviewing #5574 then #5361. If you are comfortable with the classes, #5753 is what you really want and not the other three PRs (as they are subsumed eventually by this one anway). |
|
Just to clarify, those three PRs (#5574, #5361, and #5753) build on each other. You could start with just the last one and that would hit all of the functionality. They were broken up into the littler pieces in an attempt to make it easier to review, but it is your choice how you want to tackle them. |
|
Thanks @mrambacher for the clarification. I will start with #5753. In case I need more context, I can use the preceding PRs as a reference to help understand. |
|
I am looking at your PR, and there is one thing I am not following. In Configurable::SetOptions, we already know the option name, why can't we use the name to do a point lookup in options_? Currently we iterate over all entries options_ until we find a match in the option map associated with that entry. Could you point me to an example to show how this works? Thanks |
|
Chatted offline. Thanks @mrambacher for clarifying some aspects of his design. With recursion put aside, each |
options/configurable.cc
Outdated
| input_strings_escaped); | ||
| } else { | ||
| status = ParseOption(opt_info, db_opts, opt_ptr, | ||
| name.substr(opt_iter->first.size() + 1), value, |
There was a problem hiding this comment.
If name is "test.simple.int", and opt_iter->first is "int", then name.substr(opt_iter->first.size() + 1) does not seem right.
There was a problem hiding this comment.
I do not think that can happen. If name is "test.simple.int", then opt_iter->first would either be "test.simple.int" (match completely) or "test" (matches to the first "."). This case is handling the latter, and would pass "simple.int" to the next layer to be matched.
Also note that any options prefix is stripped off before this point. So if the class had an option prefix of "test.simple" and the names passed in originally are "test.simple.int", test.simple.struct.double", and "test.bool", then the options processed here would be "int", "struct.double", and "test.bool" as the prefix would have already been removed.
There was a problem hiding this comment.
According to simple_options_info in configurable_test.cc, this can happen. Specifically in the example in configurable.cc, the opt_iter->first is "int", "bool", etc. and I believe the name can be "test.simple.int" or "test.simple.bool" if long form is used. @mrambacher
There was a problem hiding this comment.
I understand the confusion now. Here is how it works and why I do not think it is an issue:
- In ConfigureOption, the option name is turned into the short name by removing any option prefix. So "test.simple.int" becomes "int" if the class has "test.simple." as its prefix. This is meant to normalize all of the names.
- In FindOption (called from SetOption), it looks for the option name ("int" in this example) in the map. It would find the exact match and process it.
The case you are pointing out is meant to handle Configurables that have nested Configurable objects in them (has-a relationship). In this case the OptionMap would have "nested" in it and the option name passed in might be "nested" (in which case we were configuring the entire nested object) or "nested.int" (in which case we would configure the "int" property inside of "nested". (Note that since ConfigureOption stripped off the prefix, this name could have been passed in as "test.simple.nested", but the code here only sees "nested".) . In this case, the FindOption needs to find "nested" even if the option name passed in is "nested.int" (and there is code in FindOption to do exactly that). So "FindOption" matches "nested.int" and "nested" to the same option in the map. If the option passed in is "nested.int", we need to pass to the embedded option that we are configuring "int" since it has no idea that it is "nested".
Note that I think it is also possible that the nested option has a prefix on it (prefix-1.nested.prefix-2.int), in which case the "prefix-1" would be stripped off at the first level, we would look for "nested" in the map and pass "prefix-2.int" to the nested ConfigureOption and the nested class would strip off prefix-2.
@riversand963 Does that clear it up? If you think it would be helpful, I can attempt to add comments to that affect, but the comments would be de-centralized as different places are doing different parts of the processing.
There was a problem hiding this comment.
I think it is worthwhile documenting the configurable object naming you just described with examples. As you mentioned, different functions do different parts. Therefore, it may be a good idea to add a page to the Github wiki after this PR is merged.
mrambacher
force-pushed
the
ConfigurableOptions
branch
from
e543a35 to
3b52456
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.
C++ has no great way of guaranteeing the order of initialization of statics across compilation units. This was causing some ASAN issues with the introduction/extended use of class and option names (e.g. TableFactory::kBlockBasedTableName/Opts). To solve this problem, the static constant members were changed to static constant functions. With this change, there should be no "order of initialization issue" as there is no variable being initialized. By moving the kName values closer to home, some of the methods (like RegisterOptions and GetOptions) could be templatized/parameterized differently.
|
@mrambacher has updated the pull request. You must reimport the pull request before landing. |
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.
|
@mrambacher has updated the pull request. You must reimport the pull request before landing. |
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.
|
@mrambacher has updated the pull request. You must reimport the pull request before landing. |
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 7d472ac. |
Summary: This PR merges the functionality of making the ColumnFamilyOptions, TableFactory, and DBOptions into Configurable into a single PR, resolving any merge conflicts Pull Request resolved: facebook#5753 Reviewed By: ajkr Differential Revision: D23385030 Pulled By: zhichao-cao fbshipit-source-id: 8b977a7731556230b9b8c5a081b98e49ee4f160a
Summary: This PR merges the functionality of making the ColumnFamilyOptions, TableFactory, and DBOptions into Configurable into a single PR, resolving any merge conflicts Pull Request resolved: facebook#5753 Reviewed By: ajkr Differential Revision: D23385030 Pulled By: zhichao-cao fbshipit-source-id: 8b977a7731556230b9b8c5a081b98e49ee4f160a
This PR merges the functionality of making the ColumnFamilyOptions, TableFactory, and DBOptions into Configurable into a single PR, resolving any merge conflicts