Refactoring of metrics in PQRB #30617
Conversation
|
🟢 |
Run Extra TestsRun additional tests for this PR. You can customize:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors metrics handling in the PersQueue Read Balancer (PQRB) by extracting metrics-related logic into a dedicated TTopicMetricsHandler class, improving code organization and maintainability.
Key changes:
- Introduced
TTopicMetricsHandlerclass to encapsulate all metrics-related functionality - Consolidated database information into a
TDatabaseInfostruct - Removed metrics-related code from the main
TPersQueueReadBalancerclass
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| ydb/core/persqueue/public/config.h | Added forward declarations for protobuf types used in metrics handling |
| ydb/core/persqueue/pqrb/ya.make | Added new metrics implementation file to the build |
| ydb/core/persqueue/pqrb/read_balancer__metrics.h | New header defining TTopicMetricsHandler and related metrics structures |
| ydb/core/persqueue/pqrb/read_balancer__metrics.cpp | New implementation of metrics collection and aggregation logic |
| ydb/core/persqueue/pqrb/read_balancer_app.cpp | Updated HTML statistics generation to use the new metrics handler |
| ydb/core/persqueue/pqrb/read_balancer__txinit.h | Updated initialization to use TTopicMetricsHandler for partition metrics |
| ydb/core/persqueue/pqrb/read_balancer.h | Removed metrics-related fields and methods, added TDatabaseInfo struct and TTopicMetricsHandler member |
| ydb/core/persqueue/pqrb/read_balancer.cpp | Refactored to delegate metrics operations to TTopicMetricsHandler, simplified counter management |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (SplitMergeEnabled(Self->TabletConfig)) { | ||
| Self->PartitionsScaleManager = std::make_unique<TPartitionScaleManager>(Self->Topic, Self->Path, Self->DatabasePath, Self->PathId, Self->Version, Self->TabletConfig, Self->PartitionGraph); | ||
| // TODO DatabasePath is not initialized yet |
There was a problem hiding this comment.
The TODO comment states "DatabasePath is not initialized yet" but the code is actually using DatabaseInfo.DatabasePath which should be initialized. This comment seems outdated or misleading and should be removed or clarified.
| // TODO DatabasePath is not initialized yet |
|
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ ⚪ Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ ⚪ Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ ⚪ Ya make output | Test bloat | Test bloat
⚪ Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ ⚪ Ya make output | Test bloat | Test bloat
⚪ Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ ⚪ Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| TopicMetrics.TotalDataSize += dataSize; | ||
| TopicMetrics.TotalUsedReserveSize += usedReserveSize; |
There was a problem hiding this comment.
InitializePartitions accumulates DataSize and UsedReserveSize into TopicMetrics (lines 265-266), but these values will be completely replaced when UpdateMetrics is called (line 285 in UpdateMetrics method), which recalculates TopicMetrics from scratch. This means the initial accumulation in InitializePartitions is unnecessary and could be misleading. Consider removing the accumulation here or documenting that TopicMetrics will be recalculated on the first UpdateMetrics call.
| TopicMetrics.TotalDataSize += dataSize; | |
| TopicMetrics.TotalUsedReserveSize += usedReserveSize; |
| collector.Collect(partitionStatus); | ||
| } | ||
| collector.Finish(); | ||
| PartitionStatuses.erase(PartitionStatuses.begin(), PartitionStatuses.end()); |
There was a problem hiding this comment.
Using erase with range [begin, end) clears all elements from the map, but the standard approach is to simply call clear(). The current code is unnecessarily verbose and potentially confusing. Consider replacing with PartitionStatuses.clear() for better readability.
| PartitionStatuses.erase(PartitionStatuses.begin(), PartitionStatuses.end()); | |
| PartitionStatuses.clear(); |
| ++begin; | ||
| } | ||
|
|
||
| // here, the aggregation function configured in the protofile for each counter is used for each counter. |
There was a problem hiding this comment.
The comment states "the aggregation function configured in the protofile for each counter is used for each counter" which is redundant (mentions "for each counter" twice). Consider simplifying to "the aggregation function configured in the protofile is used for each counter" or "uses the aggregation function configured in the protofile for each counter".
| // here, the aggregation function configured in the protofile for each counter is used for each counter. | |
| // The aggregation function configured in the protofile is used for each counter. |
| // TODO DatabasePath is not initialized yet | ||
| Self->PartitionsScaleManager = std::make_unique<TPartitionScaleManager>(Self->Topic, Self->Path, Self->DatabaseInfo.DatabasePath, Self->PathId, Self->Version, Self->TabletConfig, Self->PartitionGraph); |
There was a problem hiding this comment.
The TPartitionScaleManager is being initialized with an empty DatabasePath. According to the TODO comment, DatabaseInfo.DatabasePath is not initialized yet at this point in TTxInit. The DatabasePath gets populated later in Handle(TEvTxProxySchemeCache::TEvWatchNotifyUpdated::TPtr&) at line 744 of read_balancer.cpp. While there is an UpdateDatabasePath method call at line 756, this PartitionsScaleManager is being initialized here with an empty path before that update occurs. Consider deferring the PartitionsScaleManager initialization until after DatabasePath is available, or ensure UpdateDatabasePath is called immediately after DatabasePath becomes available.
| // TODO DatabasePath is not initialized yet | |
| Self->PartitionsScaleManager = std::make_unique<TPartitionScaleManager>(Self->Topic, Self->Path, Self->DatabaseInfo.DatabasePath, Self->PathId, Self->Version, Self->TabletConfig, Self->PartitionGraph); | |
| // Defer PartitionsScaleManager initialization until DatabasePath is available | |
| // Initialization moved to after DatabasePath is set (see read_balancer.cpp) |
2 participants
Changelog entry
...
Changelog category
Description for reviewers
...