Add ObjectRegistry to ConfigOptions #8166
Conversation
This change enables a couple of things: - Different ConfigOptions can have different registry/factory associated with it, thereby allowing things like a "Test" ConfigOptions versus a "Production" - The ObjectRegistry is created fewer times and can be re-used The ConfigOptions can also be initialized/constructed from a DBOptions, in which case it will grab some of its settings (Env, Logger) from the DBOptions.
|
Can you fix the testing failure first? |
|
@mrambacher has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
1 similar comment
|
@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 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. |
| @@ -16,6 +16,9 @@ | |||
|
|
|||
| namespace ROCKSDB_NAMESPACE { | |||
| class Env; | |||
| class Logger; | |||
There was a problem hiding this comment.
HISTORY.md needs to be updated, explain why we need to add registry to ConfigOptions, the benefit, how to use it.
|
@mrambacher has updated the pull request. You must reimport the pull request before landing. |
| // Returns the total number of factories registered for this library. | ||
| // This method returns the sum of all factories registered for all types. | ||
| // @param num_types returns how many unique types are registered. | ||
| size_t GetFactoryCount(size_t* num_types) const; |
There was a problem hiding this comment.
Do we need it in your future PR? If so, maybe add it later in the corresponding PR.
|
@mrambacher has updated the pull request. You must reimport the pull request before landing. |
|
I do not have further comment. Remember to update HISTORY.md and I think it is good to go. |
|
@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. |
1 similar comment
|
@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 imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
2 similar comments
|
@mrambacher has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@mrambacher has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@mrambacher merged this pull request in 9f2d255. |
This change enables a couple of things:
The ConfigOptions can also be initialized/constructed from a DBOptions, in which case it will grab some of its settings (Env, Logger) from the DBOptions.