fix: [Bug] NamespaceV2 is not working#10608
Conversation
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
This PR fixes #9341 by making ClientConfig.getNamespace() fall back to namespaceV2 when the V1 namespace is not set, restoring namespace isolation for users who only configure namespaceV2.
Findings
-
[Info]
client/src/main/java/org/apache/rocketmq/client/ClientConfig.java:417-419— The fallback is correctly placed after the V1 check and before thenamesrvAddrparsing, preserving the intended priority: V1 > V2 > namesrvAddr-derived. The return is side-effect-free (does not mutatenamespaceor setnamespaceInitialized), which is consistent with how the V1 check on lines 413-415 behaves. -
[Info]
client/src/main/java/org/apache/rocketmq/client/ClientConfig.java:417-419— One subtle point: since the V2 fallback does not cache the result intonamespaceor setnamespaceInitialized = true, every call togetNamespace()will re-evaluate theStringUtils.isNotEmpty(namespaceV2)check. This is lightweight and matches the existing V1 pattern, so no action needed — just noting for awareness. -
[Info] The fix is purely client-side, which is the right approach here. Since the broker derives namespace from the resource name via
NamespaceUtil.getNamespaceFromResource, making the client correctly prefix resources with the V2 namespace value (e.g.,InstanceTest%TOPIC) is sufficient to restore isolation. TheNamespaceUtil.wrapNamespaceidempotency ensures safety when a proxy also performs its own resolution. -
[Info] Test coverage is solid — four test cases cover: basic fallback, wrapping with V2, unwrapping with V2, and V1 precedence. The idempotent wrapping test (
withNamespace("InstanceTest%resource")returning the same value) is a nice touch.
Suggestions
No blocking concerns. The change is minimal, well-targeted, and correctly addresses the root cause described in the issue.
Cross-repo Note
No changes needed in apache/rocketmq-clients — this fix is in the legacy Java client (client/ module), not the gRPC-based multi-language SDK.
Automated review by github-manager-bot
de41dce to
61197d6
Compare
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
PR: Fix: NamespaceV2 is not working
Verdict: ✅ Approved
Summary
Fixes ClientConfig.getNamespace() to fall back to namespaceV2 when namespace (v1) is not set. Previously, if a user configured only namespaceV2 without namespace, the getNamespace() method would skip namespaceV2 entirely and attempt to parse the namespace from namesrvAddr, leading to incorrect behavior.
Analysis
Correctness ✅
- The fix adds the
namespaceV2fallback in the right position — after checkingnamespacebut before falling back to parsingnamesrvAddr. - The precedence is correct:
namespace(v1) >namespaceV2>namesrvAddrparsing.
Tests ✅
- 4 new test cases covering:
- Fallback to
namespaceV2when v1 is unset withNamespace/withoutNamespaceusingnamespaceV2- V1 precedence over V2
- Fallback to
- Good coverage of the fix.
Compatibility ✅
- No breaking changes. Existing behavior with
namespaceset is unchanged. - Only affects the previously broken case where only
namespaceV2was configured.
Performance ✅
- Simple string check, no performance impact.
Verdict
Clean fix with good test coverage. Addresses a real usability gap in the namespace v2 support.
When only namespaceV2 is set, topics and consumer/producer groups are not wrapped on the client side, because ClientConfig.getNamespace() — the single value used by withNamespace/withoutNamespace and every resource wrapping call site — only returns the deprecated V1 namespace, which stays empty. namespaceV2 was intended to be resolved server-side via the nsd/ns extension fields attached by NamespaceRpcHook, but the broker never consumes those fields; instead it derives the namespace from already-wrapped resource names (NamespaceUtil.getNamespaceFromResource). With neither side wrapping the resource, producers and consumers configured with different namespaces operate on the same physical topic, so a namespace can consume messages from another namespace. Make getNamespace() fall back to namespaceV2 when the V1 namespace is unset, so that resources are wrapped with the namespaceV2 value and isolated correctly. The V1 namespace still takes precedence when both are set. Signed-off-by: 王越 <1939455790@qq.com>
61197d6 to
ffdab53
Compare
|
@lizhimins @RongtongJin gentle ping — this fixes #9341 ( The PR is |
|
Confirmed the namespace-isolation gap this addresses. The added |
Adds a regression guard for the cross-namespace isolation symptom reported in apache#9341: two clients configured with different namespaceV2 values must resolve the same resource to different fully-qualified names, so a consumer in InstanceA cannot accidentally address InstanceB's resources. The test also covers the negative case explicitly — configA.withoutNamespace on a resource resolved under configB's namespace must NOT reduce to the bare resource, confirming the prefix is namespace-pinned and the two clients stay isolated. Requested in apache#10608 code review.
|
@itxaiohanglover good catch — added the negative case you asked for in c2c4e86.
Local CI is in |
|
@itxaiohanglover done — pushed in c2c4e86. Added |
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Fixes NamespaceV2 isolation failure (#9341): when only namespaceV2 is configured (V1 namespace unset), ClientConfig.getNamespace() now falls back to namespaceV2, ensuring producers and consumers in different namespaces correctly resolve to different fully-qualified resource names.
Findings
- [Info]
ClientConfig.java:417-419— The 4-line fix is correctly placed in the precedence chain: V1namespace→ namespaceV2 → namesrvAddr parsing. V1 still takes precedence when both are set, preserving backward compatibility. - [Info]
ClientConfig.java:417— Unlike the namesrvAddr branch below, thenamespaceV2return does not cache into thenamespacefield. This is correct —namespaceV2is a direct field read (no parsing cost), and caching it intonamespacewould conflate the two distinct configuration sources. - [Info]
NamespaceUtil.wrapNamespaceis idempotent (isAlreadyWithNamespacecheck), so the fix is safe for both direct-to-broker and proxy-mediated paths.
Suggestions
- No code changes suggested. The fix is minimal and correct.
- Consider adding a
@sinceor inline comment noting thatnamespaceV2fallback was added in this PR, to help future maintainers understand the precedence chain's evolution.
Verdict
Excellent minimal fix with outstanding test coverage — especially testNamespaceV2IsolatesResourcesAcrossNamespaces which serves as a regression guard for the exact cross-namespace isolation symptom. LGTM.
Automated review by github-manager-bot
What is the purpose of the change
Fixes #9341.
NamespaceV2does not isolate resources: a consumer in one namespace can consume messages produced by a producer in a different namespace.Brief changelog
namespaceV2was designed to be resolved server-side — the client attachesnsd/nsextension fields to each remoting request throughNamespaceRpcHook, expecting the server to wrap the resource. However, the broker never reads those fields; it derives the namespace from the (already-wrapped) resource name viaNamespaceUtil.getNamespaceFromResource. Meanwhile, all client-side resource wrapping (ClientConfig.withNamespace/withoutNamespaceand their call sites in the producer/consumer) is driven byClientConfig.getNamespace(), which only returns the deprecated V1namespaceand therefore stays empty when onlynamespaceV2is configured. The net effect is that neither side wraps the resource, so producers and consumers in different namespaces hit the same physical topic.This change makes
ClientConfig.getNamespace()fall back tonamespaceV2when the V1namespaceis not set (the V1 namespace still takes precedence when both are configured). BecausegetNamespace()is the single source of truth used by every resource wrapping/unwrapping call site, topics and groups are now correctly prefixed with thenamespaceV2value, restoring isolation:namespaceV2 = InstanceTest1) →InstanceTest1%NAMESPACE_TOPICnamespaceV2 = InstanceTest) →InstanceTest%NAMESPACE_TOPICNamespaceUtil.wrapNamespaceis idempotent (isAlreadyWithNamespace), so this is safe both for the direct-to-broker path (which ignoresnsd/ns) and for a proxy that performs its own resolution.Verifying this change
Added unit tests in
client/.../ClientConfigTest:getNamespace()returnsnamespaceV2when the V1 namespace is unset.withNamespace/withoutNamespacewrap and unwrap usingnamespaceV2, and wrapping is idempotent.namespacetakes precedence overnamespaceV2when both are set.Does this pull request potentially affect one of the following parts
ClientConfig.getNamespace()now falls back tonamespaceV2for resource wrapping when the V1 namespace is unset.ClientConfigbehavior.Documentation