Copilot commented on code in PR #61318:
URL: https://github.com/apache/doris/pull/61318#discussion_r2930118313
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/CloudTabletStatMgr.java:
##########
@@ -365,4 +372,28 @@ private GetTabletStatsResponse
getTabletStats(GetTabletStatsRequest request)
public List<OlapTable.Statistics> getCloudTableStats() {
return this.cloudTableStatsList;
}
+
+ public long getTotalTableSize() {
+ return this.totalTableSize;
+ }
+
+ private void filterTopTableStatsByDataSize(List<OlapTable.Statistics>
newCloudTableStatsList) {
+ int limit = Config.prom_output_table_metrics_limit;
+ if (newCloudTableStatsList.size() <= limit) {
+ this.cloudTableStatsList = newCloudTableStatsList;
+ return;
+ }
+ // only copy elements if number of tables >
prom_output_table_metrics_limit
+ PriorityQueue<Statistics> topStats = new PriorityQueue<>(limit,
+ Comparator.comparingLong(OlapTable.Statistics::getDataSize));
+ for (OlapTable.Statistics stats : newCloudTableStatsList) {
+ if (topStats.size() < limit) {
+ topStats.offer(stats);
+ } else if (!topStats.isEmpty() && stats.getDataSize() >
topStats.peek().getDataSize()) {
+ topStats.poll();
+ topStats.offer(stats);
+ }
+ }
+ this.cloudTableStatsList =
topStats.stream().collect(Collectors.toList());
Review Comment:
`topStats.stream().collect(Collectors.toList())` adds avoidable
stream/collector overhead in a hot path (stats update daemon). `new
ArrayList<>(topStats)` is simpler and avoids extra allocations; if
deterministic ordering is desired for metric readability, sort the resulting
list explicitly (e.g., by data size descending) before publishing.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/CloudTabletStatMgr.java:
##########
@@ -290,7 +296,8 @@ private void updateStatInfo(List<Long> dbIds) {
newCloudTableStatsList.add(tableStats);
}
}
- this.cloudTableStatsList = newCloudTableStatsList;
+ filterTopTableStatsByDataSize(newCloudTableStatsList);
+ this.totalTableSize = totalTableSize;
Review Comment:
`cloudTableStatsList` and `totalTableSize` are published via two separate
volatile writes, and Prometheus output reads them via two separate calls. This
can produce inconsistent snapshots (e.g., new top-N list with previous total
size) during an update cycle. Consider publishing a single immutable snapshot
object (list + total) via one volatile reference, or at least updating both
together behind a single getter to ensure consistency for metrics consumers.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/CloudTabletStatMgr.java:
##########
@@ -365,4 +372,28 @@ private GetTabletStatsResponse
getTabletStats(GetTabletStatsRequest request)
public List<OlapTable.Statistics> getCloudTableStats() {
return this.cloudTableStatsList;
}
+
+ public long getTotalTableSize() {
+ return this.totalTableSize;
+ }
+
+ private void filterTopTableStatsByDataSize(List<OlapTable.Statistics>
newCloudTableStatsList) {
+ int limit = Config.prom_output_table_metrics_limit;
+ if (newCloudTableStatsList.size() <= limit) {
+ this.cloudTableStatsList = newCloudTableStatsList;
+ return;
+ }
+ // only copy elements if number of tables >
prom_output_table_metrics_limit
+ PriorityQueue<Statistics> topStats = new PriorityQueue<>(limit,
+ Comparator.comparingLong(OlapTable.Statistics::getDataSize));
+ for (OlapTable.Statistics stats : newCloudTableStatsList) {
Review Comment:
The method mixes the imported `Statistics` type with fully-qualified
`OlapTable.Statistics` references (e.g., `PriorityQueue<Statistics>` but
`Comparator.comparingLong(OlapTable.Statistics::getDataSize)`). For clarity and
to reduce confusion, use one style consistently (either rely on the import
everywhere, or remove the import and qualify all uses).
##########
fe/fe-core/src/main/java/org/apache/doris/metric/PrometheusMetricVisitor.java:
##########
@@ -252,30 +250,8 @@ public void visitCloudTableStats() {
StringBuilder tableRowCountBuilder = new StringBuilder();
Collection<OlapTable.Statistics> values =
tabletStatMgr.getCloudTableStats();
- // calc totalTableSize
- long totalTableSize = 0;
+ long totalTableSize = tabletStatMgr.getTotalTableSize();
Review Comment:
`visitCloudTableStats()` now reads the per-table list and the total size via
two separate calls. Since `CloudTabletStatMgr` updates these via separate
volatile writes, the Prometheus output can observe a mixed snapshot during
updates. Consider switching to a single snapshot getter (e.g., returns both
list and total) so the exported metrics are internally consistent within one
scrape.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
