Copilot commented on code in PR #59776:
URL: https://github.com/apache/doris/pull/59776#discussion_r2681427727
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/CloudTabletStatMgr.java:
##########
@@ -51,7 +49,7 @@ public class CloudTabletStatMgr extends MasterDaemon {
private static final Logger LOG =
LogManager.getLogger(CloudTabletStatMgr.class);
// <(dbId, tableId) -> OlapTable.Statistics>
- private volatile Map<Pair<Long, Long>, OlapTable.Statistics>
cloudTableStatsMap = new HashMap<>();
+ private volatile List<OlapTable.Statistics> cloudTableStatsMap = new
ArrayList<>();
Review Comment:
The variable name "cloudTableStatsMap" is misleading since it's now a List,
not a Map. Consider renaming it to something like "cloudTableStatsList" to
better reflect its actual type and avoid confusion.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/CloudTabletStatMgr.java:
##########
@@ -51,7 +49,7 @@ public class CloudTabletStatMgr extends MasterDaemon {
private static final Logger LOG =
LogManager.getLogger(CloudTabletStatMgr.class);
// <(dbId, tableId) -> OlapTable.Statistics>
Review Comment:
The comment on line 51 is outdated. It still references the old Map
structure with "(dbId, tableId) -> OlapTable.Statistics" but the field has been
changed to a List. The comment should be updated to reflect that this is now a
list of table statistics without key-based lookup.
```suggestion
// List of table statistics (OlapTable.Statistics) for all tables
```
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/CloudTabletStatMgr.java:
##########
@@ -360,7 +365,7 @@ private GetTabletStatsResponse
getTabletStats(GetTabletStatsRequest request)
return response;
}
- public Map<Pair<Long, Long>, OlapTable.Statistics> getCloudTableStatsMap()
{
+ public List<OlapTable.Statistics> getCloudTableStatsMap() {
return this.cloudTableStatsMap;
Review Comment:
The change from Map<Pair<Long, Long>, OlapTable.Statistics> to
List<OlapTable.Statistics> removes the ability to perform efficient lookups of
statistics for a specific table by its (dbId, tableId) pair. If the API was
previously used for lookups (O(1) complexity), this change would degrade
performance to O(n). However, since the only current usage in
PrometheusMetricVisitor only iterates through all entries, this change is
acceptable. Consider documenting this API change if there are external
consumers or if the method is part of a public API.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/CloudTabletStatMgr.java:
##########
@@ -360,7 +365,7 @@ private GetTabletStatsResponse
getTabletStats(GetTabletStatsRequest request)
return response;
}
- public Map<Pair<Long, Long>, OlapTable.Statistics> getCloudTableStatsMap()
{
+ public List<OlapTable.Statistics> getCloudTableStatsMap() {
Review Comment:
The method name "getCloudTableStatsMap" is misleading since it now returns a
List, not a Map. Consider renaming it to "getCloudTableStats" or
"getCloudTableStatsList" to better reflect the return type.
```suggestion
public List<OlapTable.Statistics> getCloudTableStatsList() {
```
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/CloudTabletStatMgr.java:
##########
@@ -149,20 +153,23 @@ protected void runAfterCatalogReady() {
LOG.info("finished to get tablet stat of all backends. cost: {} ms",
(System.currentTimeMillis() - start));
+ return dbIds;
+ }
+ private void updateStatInfo(List<Long> dbIds) {
// after update replica in all backends, update index row num
- start = System.currentTimeMillis();
+ long start = System.currentTimeMillis();
Pair<String, Long> maxTabletSize = Pair.of(/* tablet id= */null, /*
byte size= */0L);
Pair<String, Long> maxPartitionSize = Pair.of(/* partition id= */null,
/* byte size= */0L);
Pair<String, Long> maxTableSize = Pair.of(/* table id= */null, /* byte
size= */0L);
Pair<String, Long> minTabletSize = Pair.of(/* tablet id= */null, /*
byte size= */Long.MAX_VALUE);
Pair<String, Long> minPartitionSize = Pair.of(/* partition id= */null,
/* byte size= */Long.MAX_VALUE);
Pair<String, Long> minTableSize = Pair.of(/* tablet id= */null, /*
byte size= */Long.MAX_VALUE);
- Long totalTableSize = 0L;
- Long tabletCount = 0L;
- Long partitionCount = 0L;
- Long tableCount = 0L;
- Map<Pair<Long, Long>, OlapTable.Statistics> newCloudTableStatsMap =
new HashMap<>();
+ long totalTableSize = 0L;
+ long tabletCount = 0L;
+ long partitionCount = 0L;
+ long tableCount = 0L;
+ List<OlapTable.Statistics> newCloudTableStatsMap = new ArrayList<>();
Review Comment:
The variable name "newCloudTableStatsMap" is misleading since it's now a
List, not a Map. Consider renaming it to match the data structure, such as
"newCloudTableStatsList".
--
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]
