Make WalFilter, SstPartitionerFactory, FileChecksumGenFactory, and TableProperties Customizable #8638
Conversation
- Fix issue with OptionType::Vector when the nested item is a Customizable with no names - Fix issue with OptionType::Vector to appropriately wrap the elements in a Vector; - Fix an issue with nested Customizable object with a null immutable object still appearing in the mutable options; - Fix/Add tests for null/empty customizable objects - Move the RegisterTestObjects from customizable_test into testutil.
WalFilter, TablePropertiesCollectorFactory, SstPartitionerFactory, FileChecksumGenFactory are all now customizable and can be saved and restored from the Options file.
| @@ -68,6 +68,9 @@ static Status NewSharedObject( | |||
| if (config_options.ignore_unsupported_options && status.IsNotSupported()) { | |||
| return Status::OK(); | |||
| } | |||
| } else if (opt_map.empty()) { | |||
There was a problem hiding this comment.
Add the logic change to the comment. Previously, if id is empty, we will return not supported. But now, we will reset the result to nullptr. Also, can you explain a little bit why we need this change? Just move the logic from LoadSharedObject here?
| #include "rocksdb/utilities/customizable_util.h" | ||
|
|
||
| namespace ROCKSDB_NAMESPACE { | ||
| Status WalFilter::CreateFromString(const ConfigOptions& config_options, |
There was a problem hiding this comment.
If it is just the CreateFromString, can we move it to wal_filter.h? Declaration+definition.
There was a problem hiding this comment.
If it is just the CreateFromString, can we move it to wal_filter.h? Declaration+definition.
It should really be here. If there is a default or "shipped" implementation later, it should be registered here.
test_util/testutil.cc
Outdated
| @@ -601,5 +602,41 @@ Status CreateEnvFromSystem(const ConfigOptions& config_options, Env** result, | |||
| } | |||
| } | |||
|
|
|||
| #ifndef ROCKSDB_LITE | |||
| // This method loads existing test classes into the ObjectRegistry | |||
| int RegisterTestObjects(ObjectLibrary& library, const std::string& /*arg*/) { | |||
There was a problem hiding this comment.
Is this going to be used outside customizable_test.cc?
|
@mrambacher has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
| @@ -133,4 +134,16 @@ Status GetFileChecksumsFromManifest(Env* src_env, const std::string& abs_path, | |||
| return retriever.status(); | |||
| } | |||
|
|
|||
| Status FileChecksumGenFactory::CreateFromString( | |||
There was a problem hiding this comment.
Why don't we add the RegisterFileChecksumFactories here?
|
@mrambacher has updated the pull request. You must reimport the pull request before landing. |
|
@mrambacher 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. |
|
@mrambacher has updated the pull request. You must reimport the pull request before landing. |
|
@mrambacher has updated the pull request. You must reimport the pull request before landing. |
|
@mrambacher has updated the pull request. You must reimport the pull request before landing. |
|
@mrambacher has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
…bleProperties Customizable (#8638) Summary: Pull Request resolved: facebook/rocksdb#8638 Reviewed By: zhichao-cao Differential Revision: D31024729 Pulled By: mrambacher fbshipit-source-id: 954c04ccab0b8dee64050a27aadf78ed119106c0
No description provided.