You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@rocketmq.apache.org by "wang-jiahua (via GitHub)" <gi...@apache.org> on 2026/09/03 10:09:47 UTC
[PR] [ISSUE #3040] Fix DashboardCollectTask.fetchBrokerRuntimeStats ignoring the retry result [rocketmq-dashboard]
wang-jiahua opened a new pull request, #3041:
URL: https://github.com/apache/rocketmq-dashboard/pull/3041
### Which Issue(s) This PR Fixes
Fixes #3040
### Brief Description
`fetchBrokerRuntimeStats` retried on failure but discarded the retry's return value and unconditionally rethrew the original exception, so a successful retry changed nothing — the retries only added sleep. The catch block now propagates the retry result (`return fetchBrokerRuntimeStats(brokerAddr, retryTime - 1);`). When retries are exhausted the method already returns null (`retryTime == 0`), which the caller `collectBroker` already handles via `if (kvTable == null) continue`.
The existing `testCollectBroker` was itself evidence of the bug: it stubs the fetch to "throw once, then succeed" yet asserted that `collectBroker` throws. It has been updated to the fixed semantics (persistent failure → skip that broker without throwing), and a new test covers the transient-failure-then-success path.
### How Did You Test This Change?
- New deterministic test `testCollectBrokerRetriesFetchBrokerRuntimeStatsOnTransientFailure`: fails on the unfixed code with `RuntimeException: fetchBrokerRuntimeStats exception`, passes with the fix and asserts one collected entry.
- `DashboardCollectTaskTest` 3/3 pass on current master (JDK 17).
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #3040] Fix DashboardCollectTask.fetchBrokerRuntimeStats ignoring the retry result [rocketmq-dashboard]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #3040] Fix DashboardCollectTask.fetchBrokerRuntimeStats ignoring the retry result [rocketmq-dashboard]
Posted by "lizhimins (via GitHub)" <gi...@apache.org>.
lizhimins closed pull request #3041: [ISSUE #3040] Fix DashboardCollectTask.fetchBrokerRuntimeStats ignoring the retry result
URL: https://github.com/apache/rocketmq-dashboard/pull/3041
--
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: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org