You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@rocketmq.apache.org by "youngkermit8-coder (via GitHub)" <gi...@apache.org> on 2026/08/10 20:26:55 UTC
[PR] [ISSUE #1507] Support all Tencent Cloud RocketMQ 5.x regions [rocketmq-dashboard]
youngkermit8-coder opened a new pull request, #1508:
URL: https://github.com/apache/rocketmq-dashboard/pull/1508
## What is the purpose of the change
Tencent Cloud's RocketMQ 5.x catalog currently exposes only 10 of the 19 supported regions. This prevents Studio users from onboarding clusters in nine documented regions. Financial regions also require region-specific API endpoints because they are isolated from the non-financial nearby-access endpoint.
This closes #1507.
## Brief changelog
- Complete the Tencent Cloud RocketMQ 5.x region catalog with all 19 documented regions.
- Route `*-fsi` regions through their required regional `trocket.<region>.tencentcloudapi.com` endpoints.
- Keep the recommended nearby-access endpoint for non-financial regions.
- Add regression coverage for the complete region list and endpoint selection.
Official references:
- https://cloud.tencent.com/document/product/1493/119006
- https://cloud.tencent.com/document/product/1493/96034
## Verifying this change
- `mvn -Dtest=TencentCatalogServiceTest,TencentClientFactoryTest test`
- 5 tests passed, 0 failures, 0 errors.
- Checkstyle passed with 0 violations.
- The current base branch has unrelated compilation regressions tracked by #1502. The test command was verified with the #1502 compilation-only fixes applied locally; those unrelated changes are not included in this PR.
## Follow-up work
None.
--
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 #1507] Support all Tencent Cloud RocketMQ 5.x regions [rocketmq-dashboard]
Posted by "youngkermit8-coder (via GitHub)" <gi...@apache.org>.
youngkermit8-coder commented on PR #1508:
URL: https://github.com/apache/rocketmq-dashboard/pull/1508#issuecomment-5246088981
Addressed the remaining non-blocking review suggestions in `04f862a`:
- Added explicit coverage for `ap-shanghai-adc`, which must use the nearby-access endpoint.
- Added fallback coverage for both `null` and empty region values.
Validation:
- `TencentCatalogServiceTest` and `TencentClientFactoryTest`: 6 tests passed
- Checkstyle: 0 violations
The full main-source compilation is still blocked by the unrelated base-branch errors tracked in #1502. I will rerun the complete Maven build as soon as that fix is merged and synchronized.
--
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 #1507] Support all Tencent Cloud RocketMQ 5.x regions [rocketmq-dashboard]
Posted by "RockteMQ-AI (via GitHub)" <gi...@apache.org>.
RockteMQ-AI commented on PR #1508:
URL: https://github.com/apache/rocketmq-dashboard/pull/1508#issuecomment-5245697180
I've verified the implementation against the official Tencent Cloud docs. Here's my structured review.
## Review Summary
This PR correctly expands the Tencent Cloud RocketMQ 5.x region catalog from 10 to all 19 documented regions and routes financial (`*-fsi`) regions through their required regional API endpoints. I verified the region list and endpoint logic against the official Tencent Cloud docs (`cloud.tencent.com/document/product/1493/96034` and `/119006`) — the 19 regions, their order, and the financial-region endpoint requirement all match the authoritative source exactly. The change is correct, low-risk, and well-scoped, with one genuinely ambiguous case (`ap-shanghai-adc`) that's worth an extra test to lock in the intended behavior.
## Findings
### [LOW] Add a test asserting `ap-shanghai-adc` uses the nearby-access endpoint
- File: `server/src/test/java/org/apache/rocketmq/studio/provider/tencent/TencentClientFactoryTest.java`
- Description: `ap-shanghai-adc` (Autonomous Driving Cloud) is the one non-standard, special region in the catalog. It currently routes to `ENDPOINT` because it doesn't end in `-fsi`. This is correct per the docs (nearby-access is recommended for all non-financial regions, and ADC is not financial), but the decision is implicit and only enforced by the `-fsi` suffix heuristic. A reader could reasonably wonder whether ADC, like financial regions, is isolated and needs a regional endpoint.
- Suggestion: Add one assertion documenting the intentional decision, e.g.:
```java
assertThat(TencentClientFactory.endpointFor("ap-shanghai-adc"))
.isEqualTo(TencentClientFactory.ENDPOINT);
```
This turns the implicit assumption into a regression guard.
### [LOW] Cover the `null`/empty edge case for `endpointFor`
- File: `server/src/test/java/org/apache/rocketmq/studio/provider/tencent/TencentClientFactoryTest.java`
- Description: `endpointFor` defensively guards `region != null` (TencentClientFactory.java:90), but no test exercises that branch. In practice callers validate via `requireNonBlank`, so the null path is defensive — still, it's the one untested branch in the new logic.
- Suggestion: Add `assertThat(TencentClientFactory.endpointFor(null)).isEqualTo(TencentClientFactory.ENDPOINT);` and optionally an empty-string case for completeness.
### [INFO] `containsExactly` on the region list is correct but order-coupled
- File: `server/src/test/java/org/apache/rocketmq/studio/provider/tencent/TencentCatalogServiceTest.java` (lines 59-65)
- Description: The test asserts all 19 region IDs in exact order, which duplicates the source list. This is a strong regression test (and matches the doc's documented order), but it means any future reordering (e.g., for UX) breaks the test even though functionality is unchanged. This is an acceptable tradeoff for a curated catalog — flagging only so it's a conscious choice. If display order is not significant, `containsExactlyInAnyOrder` would be less brittle.
### [INFO] `createClient` → `endpointFor` wiring is not directly tested
- File: `server/src/main/java/org/apache/rocketmq/studio/provider/tencent/TencentClientFactory.java` (line 81)
- Description: The unit test exercises `endpointFor` in isolation. There's no test asserting that `createClient` actually calls `httpProfile.setEndpoint(endpointFor(region))` with the region-specific value. This is acceptable because the SDK's `TrocketClient` constructor is hard to mock and the wiring is a single, reviewable line — but it does mean an accidental regression (e.g., reverting to `ENDPOINT`) would only be caught by code review, not by tests.
## Verdict
**APPROVE**
The implementation is correct and matches the official Tencent Cloud documentation verbatim (all 19 regions, financial-region regional endpoints `trocket.ap-shanghai-fsi.tencentcloudapi.com` / `trocket.ap-shenzhen-fsi.tencentcloudapi.com`, nearby-access for non-financial including ADC). Performance is unchanged (immutable static list, per-region cached clients, trivial `endpointFor`), and backward compatibility is preserved (non-`fsi` regions keep the existing endpoint; `fsi` regions are newly added so no existing users are affected). The findings above are non-blocking polish — the ADC and null-case tests are the most worthwhile additions.
Note: I did not run the test suite because the PR notes unrelated base-branch compilation regressions tracked by #1502; my assessment is based on static analysis of the diff and verification against the cited Tencent Cloud docs.
Sources:
- [Supported Regions (消息队列 RocketMQ 版)](https://cloud.tencent.com/document/product/1493/96034)
- [Trocket API endpoints / 金融区接入](https://cloud.tencent.com/document/product/1493/119006)
--
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 #1507] Support all Tencent Cloud RocketMQ 5.x regions [rocketmq-dashboard]
Posted by "lizhimins (via GitHub)" <gi...@apache.org>.
lizhimins merged PR #1508:
URL: https://github.com/apache/rocketmq-dashboard/pull/1508
--
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 #1507] Support all Tencent Cloud RocketMQ 5.x regions [rocketmq-dashboard]
Posted by "youngkermit8-coder (via GitHub)" <gi...@apache.org>.
youngkermit8-coder commented on PR #1508:
URL: https://github.com/apache/rocketmq-dashboard/pull/1508#issuecomment-5246149449
Additional isolated validation: I applied this PR's two commits on top of #1502 (`c00a5ef`) in a clean worktree and ran:
```text
mvn -B -ntp -Dtest=TencentCatalogServiceTest,TencentClientFactoryTest clean package
Compiling 374 main source files
Compiling 110 test source files
Tests run: 6, Failures: 0, Errors: 0, Skipped: 0
Checkstyle: 0 violations
BUILD SUCCESS
```
This confirms the PR compiles, tests, and packages cleanly with the pending base-build fix, without adding #1502's unrelated changes to this branch.
--
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