Skip to content

Support client properties in Java client#1237

Open
imjyz wants to merge 3 commits into
apache:masterfrom
imjyz:feat/support-client-properties-java
Open

Support client properties in Java client#1237
imjyz wants to merge 3 commits into
apache:masterfrom
imjyz:feat/support-client-properties-java

Conversation

@imjyz
Copy link
Copy Markdown

@imjyz imjyz commented Apr 29, 2026

Which Issue(s) This PR Fixes

#1236

Brief Description

Add clientProperties to Settings to support capabilities similar to Pulsar producer.properties and consumer.properties.

Copy link
Copy Markdown
Contributor

@qianye1001 qianye1001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! Here are some review comments:

1. computeClientPropertySize may not align with proto's "Total serialized size <= 4KB"

private static int computeClientPropertySize(String key, String value) {
    return key.getBytes(StandardCharsets.UTF_8).length + value.getBytes(StandardCharsets.UTF_8).length;
}

The proto definition states "Total serialized size: <= 4KB". Protobuf's wire format for map<string, string> includes per-entry overhead (field tags, length varints). The current implementation only sums raw key+value bytes, which is smaller than the actual serialized size. This means a payload that passes client-side validation could still be rejected by the server with INVALID_CLIENT_PROPERTIES.

Suggestion: either align the calculation with protobuf wire format overhead, or document explicitly that this is a lenient client-side pre-check and the server has final authority.

2. Unnecessary byte array allocation for key size

The key pattern [a-zA-Z][a-zA-Z0-9_.-]* guarantees ASCII-only characters, so UTF-8 byte length always equals key.length(). No need to allocate a byte[] via key.getBytes(UTF_8).length. (For values, getBytes is appropriate since they may contain non-ASCII.)

3. Missing PublishingSettings serialization test for clientProperties

PushSubscriptionSettingsTest and SimpleSubscriptionSettingsTest both assert clientProperties in toProtobuf() output, but there's no equivalent test for PublishingSettings. The fakeProducerSettings() in TestBase passes Collections.emptyMap(), which doesn't exercise the serialization path.

4. Example code should not suggest specific key semantics

.addClientProperty("app", "yourAppName")
.addClientProperty("env", "yourEnvironment")

RocketMQ has no built-in understanding of "app" or "env". Putting these in examples implies the broker interprets or acts on these keys, which it doesn't — they are opaque metadata for observability only. The choice of property keys belongs to the user's operational platform, not the MQ SDK.

Suggestion: use generic placeholders like addClientProperty("key1", "value1"), or comment them out with // like other optional configurations. If the community wants to provide operational best practices for key naming, that belongs in a separate operations guide, not embedded in SDK example code.

Similarly, the "Recommended keys: app, env, region, zone, version, instance, owner, deployment" in the proto comments (rocketmq-apis#108) might benefit from being rephrased to "Example keys used by some organizations" to avoid implying these are MQ-level conventions.

5. Minor: redundant validation in build()

build() calls validateClientProperties(clientProperties) with full entry-level validation. If all entries were added via addClientProperty, they were already validated individually. This is defense-in-depth (not a bug), but a brief comment explaining the intent would help readers.

@imjyz
Copy link
Copy Markdown
Author

imjyz commented May 20, 2026

Done. Optimized as suggested, please take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants