common: introduce md_config_cacher_t #20320
Conversation
Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>
src/osd/PrimaryLogPG.cc
Outdated
| const static md_config_cacher_t<uint64_t> osd_max_object_size( | ||
| *g_conf, "osd_max_object_size"); | ||
| const static md_config_cacher_t<bool> conf_data_digest( | ||
| *g_conf, "osd_skip_data_digest"); |
There was a problem hiding this comment.
If these are static the lifecycle is not quite right. (For example, these will outlive the CephContext and md_config_t itself.) Instead, let's make these members of OSDService
There was a problem hiding this comment.
i think this is a good strategy!
we'll want to keep an eye on the total number of these objects, since md_config_t::_apply_changes() is linear with respect the number of observers. but i think this makes the right tradeoff in terms of optimizing reads vs infrequent writes
src/osd/PrimaryLogPG.cc
Outdated
| const static md_config_cacher_t<uint64_t> osd_max_object_size( | ||
| *g_conf, "osd_max_object_size"); | ||
| const static md_config_cacher_t<bool> conf_data_digest( | ||
| *g_conf, "osd_skip_data_digest"); |
There was a problem hiding this comment.
nit: cct->_conf instead of g_conf
| } | ||
|
|
||
| ~md_config_cacher_t() { | ||
| conf.remove_observer(this); |
There was a problem hiding this comment.
object lifetimes are going to get tricky here, especially if we're declaring these as static
one option is to require md_config_cacher_t to hold a reference to CephContext. another is to skip this call to remove_observer() for static variables, as ~md_config_t() doesn't appear to care whether all observers were safely removed
There was a problem hiding this comment.
I'd prefer to be pedantic about lifecycle, because in many case we want ot ensure the cct gets destroy (e.g., library code) and using static would prevent that from ever happening. It shouldn't be hard to attach instances of this to a class that gets cleaned up before cct (i.e., something that owns a ref to the cct).
| class md_config_cacher_t : public md_config_obs_t { | ||
| md_config_t& conf; | ||
| const char* const option_name; | ||
| std::atomic<ValueT> value_cache; |
There was a problem hiding this comment.
a bit surprising, if nothing constrains the type of ValueT
There was a problem hiding this comment.
Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>
rzarzynski
force-pushed
the
wip-common-md_config_cacher_t
branch
from
651ae84
to
248750f
Compare
No description provided.