-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add estimate_pending_compaction_bytes metric to analyze fragmentation rate #1736
feat: add estimate_pending_compaction_bytes metric to analyze fragmentation rate #1736
Conversation
…tation rate Add estimate_pending_compaction_bytes metric to analyze fragmentation rate. Fixes: OpenAtomFoundation#1665 Signed-off-by: yaoyinnan <[email protected]>
73c2438
to
f29e8c1
Compare
@@ -136,6 +136,9 @@ void Redis::GetRocksDBInfo(std::string &info, const char *prefix) { | |||
write_stream_key_value(rocksdb::DB::Properties::kTotalSstFilesSize, "total_sst_files_size"); | |||
write_stream_key_value(rocksdb::DB::Properties::kLiveSstFilesSize, "live_sst_files_size"); | |||
|
|||
// pending compaction bytes | |||
write_stream_key_value(rocksdb::DB::Properties::kEstimatePendingCompactionBytes, "estimate_pending_compaction_bytes"); | |||
|
|||
// block cache | |||
write_stream_key_value(rocksdb::DB::Properties::kBlockCacheCapacity, "block_cache_capacity"); | |||
write_stream_key_value(rocksdb::DB::Properties::kBlockCacheUsage, "block_cache_usage"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the code patch provided, here are some observations and suggestions:
Observations:
- The code is adding a new line to write the "estimate_pending_compaction_bytes" property to the info stream.
- It appears that this code is part of a larger function called
GetRocksDBInfo
. - No other specific details about the overall function or the context in which this code is used are provided.
Suggestions:
-
Confirm Dependency: Ensure that you have verified that the
rocksdb::DB::Properties::kEstimatePendingCompactionBytes
property is available and works as expected in your version of RocksDB. -
Error Handling: It's important to handle any potential errors that may occur during the process of retrieving and writing the properties. Make sure appropriate error handling mechanisms (e.g., try-catch) are implemented.
-
Code Documentation: Consider adding comments to describe the purpose of this function and the significance of each property being written to the info stream.
-
Review Other Properties: Since this code patch only includes the addition of the "estimate_pending_compaction_bytes" property, it's worth considering if there are any other relevant properties that should be included in the info stream.
-
Unit Testing: Test the functionality of this code patch thoroughly to ensure it produces the desired results and doesn't introduce any unexpected issues. Verify that the info stream contains all the expected properties and their corresponding values.
Remember that without additional context and a complete understanding of your codebase and requirements, it's difficult to provide a comprehensive review.
ValueName: "estimate_pending_compaction_bytes", | ||
}, | ||
}, | ||
|
||
// snapshots | ||
"num_snapshots": { | ||
Parser: ®exParser{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code patch you provided adds a new metric configuration for "estimate_pending_compaction_bytes" to the existing collectRocksDBMetrics
map. Here's a brief code review:
-
Syntax and Formatting: The code appears to be formatted correctly, with indentation and line breaks.
-
Regular Expression: The regular expression used in the
reg
field of theregexParser
struct seems fine. However, it's always good to thoroughly test a regular expression to ensure it matches the intended patterns. -
MetaData Configuration: The
MetricMeta
struct defines metadata for the new metric. It includes the metric name, help description, type (presumably "gauge"), labels, and value name. As long as these fields are accurate and appropriate for your use case, the configuration seems fine.
Overall, from the given code patch, there don't appear to be any obvious bugs or risks. However, it's essential to thoroughly test the code to ensure proper functionality. Additionally, it's challenging to provide further improvement suggestions without more context about the codebase and its requirements.
…tation rate (OpenAtomFoundation#1736) Add estimate_pending_compaction_bytes metric to analyze fragmentation rate. Fixes: OpenAtomFoundation#1665 Signed-off-by: yaoyinnan <[email protected]>
…tation rate (OpenAtomFoundation#1736) Add estimate_pending_compaction_bytes metric to analyze fragmentation rate. Fixes: OpenAtomFoundation#1665 Signed-off-by: yaoyinnan <[email protected]>
Add estimate_pending_compaction_bytes metric to analyze fragmentation rate.
Fixes: #1665