RockteMQ-AI commented on code in PR #1253:
URL:
https://github.com/apache/rocketmq-dashboard/pull/1253#discussion_r3737331527
##########
server/src/main/java/org/apache/rocketmq/studio/cluster/broker/RealClusterProvider.java:
##########
@@ -67,7 +68,71 @@ public ClusterVO refreshClusterDetail(String clusterId) {
if (namesrvAddr == null || namesrvAddr.isBlank()) {
throw new BusinessException(400, "No NameServer configured for
cluster " + clusterId);
}
- return describeCluster(namesrvAddr);
+ return describeClusters(namesrvAddr).stream()
Review Comment:
**[Performance]** `refreshClusterDetail()` now calls `describeClusters()`
which fetches ALL clusters from the NameServer, then filters by ID. For a
dashboard with many clusters, this is less efficient than the previous direct
lookup.
Consider keeping a dedicated single-cluster lookup path for
`refreshClusterDetail()`, or cache the result if this endpoint is called
frequently.
Not blocking — acceptable for a dashboard UI operation.
##########
server/src/main/java/org/apache/rocketmq/studio/cluster/broker/RealClusterProvider.java:
##########
@@ -67,7 +68,71 @@ public ClusterVO refreshClusterDetail(String clusterId) {
if (namesrvAddr == null || namesrvAddr.isBlank()) {
throw new BusinessException(400, "No NameServer configured for
cluster " + clusterId);
}
- return describeCluster(namesrvAddr);
+ return describeClusters(namesrvAddr).stream()
+ .filter(cluster -> clusterId.equals(cluster.getId()))
+ .findFirst()
+ .orElseThrow(() -> new BusinessException(404, "Cluster not
found: " + clusterId));
+ }
+
+ /**
+ * Maps the live topology into one {@link ClusterVO} per cluster known to
the NameServer.
+ * Brokers are grouped by their owning cluster, so a NameServer that
manages several clusters
+ * no longer lumps every broker into a single cluster.
+ */
+ public List<ClusterVO> describeClusters(String namesrvAddr) {
+ return adminFactory.execute(namesrvAddr, null,
+ admin -> toClusterVOs(namesrvAddr,
admin.examineBrokerClusterInfo()));
+ }
+
+ private List<ClusterVO> toClusterVOs(String namesrvAddr, ClusterInfo
clusterInfo) {
+ Map<String, BrokerData> brokerAddrTable =
+ clusterInfo.getBrokerAddrTable() == null ? Map.of() :
clusterInfo.getBrokerAddrTable();
+ Map<String, Set<String>> clusterAddrTable =
+ clusterInfo.getClusterAddrTable() == null ? Map.of() :
clusterInfo.getClusterAddrTable();
+ List<NameServerVO> nameServers =
Arrays.stream(namesrvAddr.split("[;,]"))
+ .map(String::trim)
+ .filter(addr -> !addr.isEmpty())
+ .map(addr ->
NameServerVO.builder().addr(addr).status(ClusterStatus.healthy).build())
Review Comment:
**[Suggestion]** The broker sorting comparator is duplicated in two places
within `toClusterVOs()` (lines 95 and 108). Consider extracting it to a
constant:
```java
private static final Comparator<BrokerVO> BY_NAME =
Comparator.comparing(BrokerVO::getName,
Comparator.nullsLast(Comparator.naturalOrder()));
```
Then use `.sorted(BY_NAME)` in both places.
--
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]
