(LedgerStore) Move metric sample counters out from LedgerColumnOptions #25042
Conversation
|
The PR needs some update as we changed the direction to a rate-limiting approach. Put this PR back to draft. |
Codecov Report
@@ Coverage Diff @@
## master #25042 +/- ##
========================================
Coverage 82.0% 82.0%
========================================
Files 598 602 +4
Lines 165882 166830 +948
========================================
+ Hits 136125 136931 +806
- Misses 29757 29899 +142 |
ledger/src/blockstore_db.rs
Outdated
| @@ -338,6 +340,7 @@ impl Rocks { | |||
| access_type: access_type.clone(), | |||
| oldest_slot, | |||
| column_options, | |||
| write_perf_status: BlockstoreRocksDbPerfSamplingStatus::default(), | |||
There was a problem hiding this comment.
nit: rename to "write_batch_perf_status" to help clarify that this is for the WriteBatch codepath
| @@ -511,6 +512,13 @@ pub(crate) fn report_rocksdb_write_perf(metric_header: &'static str) { | |||
| }); | |||
| } | |||
|
|
|||
| #[derive(Debug, Default, Clone)] | |||
| /// A struct that holds the current status of RocksDB perf sampling. | |||
| pub struct BlockstoreRocksDbPerfSamplingStatus { | |||
There was a problem hiding this comment.
Such long names actually reduce readability of the code.
Within this module and blockstore module it is clear that this is blockstore specific.
outside this module, this is already qualified by the name of the module:
blockstore_metrics::BlockstoreRocksDbPerfSamplingStatus
So there is no need to repeat Blockstore twice.
Also there is no use for mentioning RocksDb in the name. Does this struct or user of this struct care if the backend is rocks-db or something else? Should we change this struct if backend changes?
…omicUsize (#25043) #### Problem After #25042, each LedgerColumn has its own BlockstoreRocksDbWritePerfMetrics and BlockstoreRocksDbReadPerfMetrics instances. As it has total ownership, its member field does not need to use Arc. #### Summary of Changes Change perf_samples_counter from Arc<AtomicUsize> to AtomicUsize under BlockstoreRocksDbWritePerfMetrics and BlockstoreRocksDbReadPerfMetrics.
Problem
LedgerColumnOptions contain two fields, perf_read_counter and perf_write_counter,
that are not really options but internal counters.
Summary of Changes
This PR introduces BlockstoreRocksDbPerfSamplingStatus, a struct that holds internal
status for RocksDB perf sampling and moves perf_read_counter and perf_write_counter
out from LedgerColumnOptions.