You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@rocketmq.apache.org by "Aias00 (via GitHub)" <gi...@apache.org> on 2026/08/03 15:17:30 UTC
[PR] [ISSUE #10801] Surface unexpected topic route lookup errors [rocketmq]
Aias00 opened a new pull request, #10802:
URL: https://github.com/apache/rocketmq/pull/10802
## Summary
- make DefaultAdminService.topicExist return false only for explicit topic-not-found errors
- surface unexpected NameServer route lookup failures instead of silently treating them as missing topics
- add regression coverage for not-found and unexpected failures
Fixes #10801
## Tests
- JAVA_HOME=/Library/Java/JavaVirtualMachines/zulu-8.jdk/Contents/Home mvn -pl proxy -Dtest=DefaultAdminServiceTest test
- git diff --check
--
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 #10801] Surface unexpected topic route lookup errors [rocketmq]
Posted by "Aias00 (via GitHub)" <gi...@apache.org>.
Aias00 closed pull request #10802: [ISSUE #10801] Surface unexpected topic route lookup errors
URL: https://github.com/apache/rocketmq/pull/10802
--
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 #10801] Surface unexpected topic route lookup errors [rocketmq]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10802:
URL: https://github.com/apache/rocketmq/pull/10802#issuecomment-5178467596
## [Codecov](https://app.codecov.io/gh/apache/rocketmq/pull/10802?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 48.25%. Comparing base ([`eddb235`](https://app.codecov.io/gh/apache/rocketmq/commit/eddb235c1bd8adc5c3a1125a294405526e7bb0e3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`c0fac7f`](https://app.codecov.io/gh/apache/rocketmq/commit/c0fac7f1b24c0dcf2c85bf83710e8a590b05c478?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on develop.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## develop #10802 +/- ##
=============================================
- Coverage 48.34% 48.25% -0.10%
+ Complexity 13527 13495 -32
=============================================
Files 1380 1380
Lines 101104 101140 +36
Branches 13107 13121 +14
=============================================
- Hits 48882 48802 -80
- Misses 46267 46346 +79
- Partials 5955 5992 +37
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/rocketmq/pull/10802?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
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 #10801] Surface unexpected topic route lookup errors [rocketmq]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #10802:
URL: https://github.com/apache/rocketmq/pull/10802#discussion_r3705402347
##########
proxy/src/test/java/org/apache/rocketmq/proxy/service/admin/DefaultAdminServiceTest.java:
##########
@@ -87,6 +88,22 @@ public void testCreateTopic() throws Exception {
assertEquals(8, topicConfigArgumentCaptor.getValue().getReadQueueNums());
}
+ @Test
+ public void testTopicExistReturnsFalseForNotFound() throws Exception {
+ when(mqClientAPIExt.getTopicRouteInfoFromNameServer(eq("missingTopic"), anyLong()))
+ .thenThrow(new MQClientException(ResponseCode.TOPIC_NOT_EXIST, "topic not exist"));
+
+ assertFalse(defaultAdminService.topicExist("missingTopic"));
+ }
+
+ @Test(expected = IllegalStateException.class)
+ public void testTopicExistThrowsForUnexpectedRouteLookupFailure() throws Exception {
+ when(mqClientAPIExt.getTopicRouteInfoFromNameServer(eq("brokenTopic"), anyLong()))
+ .thenThrow(new MQClientException(ResponseCode.SYSTEM_ERROR, "namesrv unavailable"));
+
+ defaultAdminService.topicExist("brokenTopic");
+ }
Review Comment:
Using `@Test(expected = ...)` only asserts the exception type and does not verify the exception message/cause. Since this PR is specifically about surfacing unexpected lookup failures, it would be stronger to assert that the thrown `IllegalStateException` preserves the original `MQClientException` as the cause (and optionally that the message contains the topic). Prefer `assertThrows` + assertions on `getCause()`/`getMessage()`.
##########
proxy/src/main/java/org/apache/rocketmq/proxy/service/admin/DefaultAdminService.java:
##########
@@ -49,8 +49,12 @@ public boolean topicExist(String topic) {
try {
topicRouteData = this.getTopicRouteDataDirectlyFromNameServer(topic);
topicExist = topicRouteData != null;
- } catch (Throwable e) {
- topicExist = false;
+ } catch (Exception e) {
+ if (TopicRouteHelper.isTopicNotExistError(e)) {
+ topicExist = false;
+ } else {
+ throw new IllegalStateException("get topic route " + topic + " failed", e);
Review Comment:
The thrown error message is missing quoting/context around the topic name, which can make debugging harder if the topic contains whitespace/special characters. Consider formatting it more explicitly (e.g., include quotes or a key/value style like `topic='...'`) to improve log searchability and readability.
--
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 #10801] Surface unexpected topic route lookup errors [rocketmq]
Posted by "Aias00 (via GitHub)" <gi...@apache.org>.
Aias00 commented on PR #10802:
URL: https://github.com/apache/rocketmq/pull/10802#issuecomment-5301194218
Consolidated into #10773; both changes harden DefaultAdminService route lookup behavior.
--
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