Support client properties in Java client#1237
Conversation
qianye1001
left a comment
There was a problem hiding this comment.
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.
|
Done. Optimized as suggested, please take a look. |
Which Issue(s) This PR Fixes
#1236
Brief Description
Add clientProperties to Settings to support capabilities similar to Pulsar producer.properties and consumer.properties.