Add Support for saving CompressionOptions to Options File #6817
Conversation
|
@mrambacher Thanks for the fix. We will hold it until 6424, 6425 are merged. |
mrambacher
force-pushed
the
CompressionOpts
branch
from
e659c71
to
3b83a9d
Compare
|
Thanks for working on the CompressionOption part the dealing with the special case! Looks good to me. Just some minor comments. |
|
@siying You may take a look at this PR if it is as you expected. |
There was a problem hiding this comment.
It looks good to me. Maybe @ajkr has some comments. But I would defer to @zhichao-cao to approve.
Thanks for reviewing it. @ajkr you may take a look before I approve it? |
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.
| compression_opts.enabled = | ||
| ParseBoolean("", value.substr(start, value.size() - start)); | ||
| } | ||
| } | ||
|
|
||
| // enabled is optional for backwards compatibility |
There was a problem hiding this comment.
Maybe we should document somewhere that enabled is actually required if the user tries to provide parallel_threads. It seems a bit difficult to figure out right now.
There was a problem hiding this comment.
Maybe we should document somewhere that
enabledis actually required if the user tries to provideparallel_threads. It seems a bit difficult to figure out right now.
You mean, explain it in the HISTORY.md?
|
|
||
| ASSERT_OK(GetColumnFamilyOptionsFromString( | ||
| config_options, ColumnFamilyOptions(), | ||
| "compression_opts=4:5:6:7:8:9:true; " |
There was a problem hiding this comment.
I wonder whether "compression_opts=4:5:6:7:8:9:2" works, as that seems the most likely mistake (trying to specify 2 for parallel_threads but actually setting enabled).
There was a problem hiding this comment.
That actually would not work, since "2" is not a valid boolean. That case would throw an exception and return an error.
As it is written, if you want to specify parallel_threads, you must also specify enabled. The reason for this is that enabled was added in an earlier release.
This change allows the CompressionOptions to be read/written to the options file. The CompressionOptions can be read as a ":" separated list or as a name=value structure. Additionally, because the option is a struct, indvidual elements (compression_opts.window_bits) can also be read/written. The parallel threads option is also now supported in the ":" format. Tests were added that the optional arguments in the ":" format are parsed successfully.
mrambacher
force-pushed
the
CompressionOpts
branch
from
a3f7fd1
to
18108f7
Compare
|
@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 ec711b2. |
This PR does a few things:
The compression options are now stored and treated as a OptionTypeInfo::Struct by the options system, meaning they can be read and written like the other structs. This change allows them to be read/written easily to the options file.
Additionally, the colon-format was extended to allow support for setting parallel threads. Tests were added to test all of the option settings via the optional parameters in the colon format.