Log CompactOnDeletionCollectorFactory parameters on DB open #6686
Conversation
| @@ -38,6 +38,8 @@ class CompactOnDeletionCollectorFactory | |||
| return "CompactOnDeletionCollector"; | |||
| } | |||
|
|
|||
| virtual std::string ToString() const override; | |||
There was a problem hiding this comment.
Minor: could you please change this and all overriding methods (including the destructor) so that virtual is not explicitly specified but override is?
There was a problem hiding this comment.
I think this class is extendable - specifically, MyRocks is planning to extend and override some methods so they can track such compactions.
There was a problem hiding this comment.
I'm not saying these methods shouldn't be virtual; it's just a coding style comment (see https://google.github.io/styleguide/cppguide.html#Inheritance). override in the derived class implies virtual (the code does not compile if it doesn't override anything), so virtual in the derived class is superfluous. (In the base class, virtual is of course necessary.)
include/rocksdb/table_properties.h
Outdated
|
|
||
| // Can be overridden by sub-classes to return configuration info that will | ||
| // be logged to the info log when the DB is opened | ||
| virtual std::string ToString() const { return ""; } |
There was a problem hiding this comment.
Since this does not include the name, I'm wondering if something like DebugInfo, Parameters or Configuration would be a better name. Alternatively, we could keep ToString and make it include the name as well, in which case by default it could return what Name returns (as opposed to an empty string).
There was a problem hiding this comment.
I like the latter idea.
options/options.cc
Outdated
| @@ -321,7 +321,12 @@ void ColumnFamilyOptions::Dump(Logger* log) const { | |||
| compaction_options_fifo.allow_compaction); | |||
| std::string collector_names; | |||
| for (const auto& collector_factory : table_properties_collector_factories) { | |||
| std::string collector_config; | |||
There was a problem hiding this comment.
We could generate the entire string that gets logged using a single std::ostringstream. I think that would be cleaner and a bit more efficient. (Also, collector_names is a bit of a misnomer now since we log the entire collector configuration.)
| @@ -78,6 +78,12 @@ CompactOnDeletionCollectorFactory::CreateTablePropertiesCollector( | |||
| sliding_window_size_.load(), deletion_trigger_.load()); | |||
| } | |||
|
|
|||
| std::string CompactOnDeletionCollectorFactory::ToString() const { | |||
| return "Sliding window size = " + | |||
There was a problem hiding this comment.
We could consider using std::ostringstream here as well.
There was a problem hiding this comment.
@anand1976 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@anand1976 merged this pull request in 3d6d7bc. |
Summary: Log it in the info log to help in troubleshooting. It is logged as follows - ``` 2020/04/10-10:51:39.886662 7ffff7fef340 Options.table_properties_collectors: CompactOnDeletionCollector (Sliding window size = 100 Deletion trigger = 90); ``` Tests: make check Pull Request resolved: #6686 Reviewed By: ltamasi Differential Revision: D21002442 Pulled By: anand1976 fbshipit-source-id: 7adf0dbae7f1febcb00ce61fea5097118ede5c6a
Log it in the info log to help in troubleshooting. It is logged as follows -
Tests:
make check