Copilot commented on code in PR #3041:
URL:
https://github.com/apache/rocketmq-dashboard/pull/3041#discussion_r3923548129
##########
src/test/java/org/apache/rocketmq/dashboard/task/DashboardCollectTaskTest.java:
##########
@@ -199,6 +198,27 @@ public void testCollectBroker() throws Exception {
Assert.assertEquals(brokerData.get("broker-a" + ":" +
MixAll.MASTER_ID).size(), taskExecuteNum + 2);
}
+ @Test
+ public void
testCollectBrokerRetriesFetchBrokerRuntimeStatsOnTransientFailure() throws
Exception {
+ when(rmqConfigure.isEnableDashBoardCollect()).thenReturn(true);
+ HashMap<String, String> result = new HashMap<>();
+ result.put("getTotalTps", "0.0 0.033330000333300004
0.03332972261338355");
+ KVTable kvTable = new KVTable();
+ kvTable.setTable(result);
+ // The first fetch fails transiently; fetchBrokerRuntimeStats is
expected to retry and
+ // return the successful result. Before the fix, the retry's return
value was ignored and
+ // the original exception was always rethrown, so collectBroker threw
and collected nothing.
+ when(mqAdminExt.fetchBrokerRuntimeStats(anyString()))
+ .thenThrow(new RuntimeException("fetchBrokerRuntimeStats
exception"))
+ .thenReturn(kvTable);
+
+ dashboardCollectTask.collectBroker();
+
+ LoadingCache<String, List<String>> map =
dashboardCollectService.getBrokerMap();
+ Assert.assertEquals(1, map.size());
+ Assert.assertEquals(1, map.get("broker-a" + ":" +
MixAll.MASTER_ID).size());
Review Comment:
These assertions hard-code that exactly one broker entry exists in the
cache. If `collectBroker()` is configured to iterate multiple brokers in this
test fixture, the stubbed `.thenReturn(kvTable)` will likely allow subsequent
broker fetches to succeed too, making `map.size()` > 1 and causing a brittle
test. Consider asserting only the expected key/value behavior for `broker-a`
(e.g., `map.getIfPresent(key)` and its list size), or configuring the test
fixture to include only the single broker you want to exercise.
##########
src/main/java/org/apache/rocketmq/dashboard/task/DashboardCollectTask.java:
##########
@@ -156,9 +156,9 @@ private KVTable fetchBrokerRuntimeStats(String brokerAddr,
Integer retryTime) {
Throwables.throwIfUnchecked(e1);
throw new RuntimeException(e1);
}
- fetchBrokerRuntimeStats(brokerAddr, retryTime - 1);
- Throwables.throwIfUnchecked(e);
- throw new RuntimeException(e);
+ // Return the retry result so a successful retry actually takes
effect. When all retries
+ // are exhausted this returns null, which the caller already
handles by skipping the broker.
+ return fetchBrokerRuntimeStats(brokerAddr, retryTime - 1);
Review Comment:
If the retry sleep is interrupted, the code throws without restoring the
thread’s interrupted flag. For correct interrupt handling (especially if this
task runs on a scheduler/executor), set `Thread.currentThread().interrupt()` in
the `InterruptedException` path before returning/throwing, and consider
avoiding `Throwables.throwIfUnchecked(e1)` here since `InterruptedException` is
checked and the helper is effectively redundant.
##########
src/test/java/org/apache/rocketmq/dashboard/task/DashboardCollectTaskTest.java:
##########
@@ -172,16 +172,15 @@ public void testCollectBroker() throws Exception {
KVTable kvTable = new KVTable();
kvTable.setTable(result);
when(mqAdminExt.fetchBrokerRuntimeStats(anyString()))
- .thenThrow(new RuntimeException("fetchBrokerRuntimeStats
exception"))
+ .thenThrow(new RuntimeException("fetchBrokerRuntimeStats
exception"),
+ new RuntimeException("fetchBrokerRuntimeStats
exception"),
+ new RuntimeException("fetchBrokerRuntimeStats
exception"))
.thenReturn(kvTable);
Review Comment:
This stub contradicts the intent described below (\"All retries ... fail\"),
because it will start returning `kvTable` if `fetchBrokerRuntimeStats` is
invoked more times (e.g., if the retry count changes, or additional brokers are
fetched). To keep the test stable and intent-aligned, stub it to consistently
fail for this scenario (e.g., remove the `.thenReturn(kvTable)` or use an
answer that always throws), or explicitly match the number of attempts to the
configured retry count.
--
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]
