[ISSUE #5364]✨Enhance ClientConfig with new trace and metadata pagination settings #5365
Conversation
rocketmq-rust-bot
requested review from
SpaceXCN,
TeslaRustor and
rocketmq-rust-bot
|
🔊@mxsm 🚀Thanks for your contribution🎉！ 💡CodeRabbit(AI) will review your code first🔥！ Note 🚨The code review suggestions from CodeRabbit are to be used as a reference only, and the PR submitter can decide whether to make changes based on their own judgment. Ultimately, the project management personnel will conduct the final code review💥. |
rocketmq-rust-bot
added
AI review first
WalkthroughThis PR enhances ClientConfig with two new configuration fields (trace_msg_batch_num and max_page_size_in_get_metadata), adds comprehensive getter/setter methods for all internal fields, introduces utility methods for cloning and resetting configurations, improves safety in client ID building logic, and marks the without_namespace method as deprecated. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
rocketmq-client/src/base/client_config.rs (2)
203-213: Capacity estimate could be more accurate whenclient_ipisNone.The pre-allocation reserves 15 bytes for
client_ipeven when it'sNone, but line 211-213 shows nothing is pushed in that case. This is a minor inefficiency, not a bug.🔎 Suggested improvement
- let estimated_capacity = self.client_ip.as_ref().map(|ip| ip.len()).unwrap_or(15) + let estimated_capacity = self.client_ip.as_ref().map(|ip| ip.len()).unwrap_or(0)
541-544: Consider removing redundant wrapper.
clone_client_configsimply delegates toself.clone(). SinceClientConfigderivesClone, callers can call.clone()directly. This wrapper adds no value.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rocketmq-client/src/base/client_config.rs
🧰 Additional context used
🧬 Code graph analysis (1)
rocketmq-client/src/base/client_config.rs (1)
rocketmq-remoting/src/protocol/namespace_util.rs (1)
without_namespace_with_namespace(50-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build & Test (windows-latest)
- GitHub Check: Build & Test (ubuntu-latest)
- GitHub Check: Code Coverage
- GitHub Check: Build & Test (macos-latest)
- GitHub Check: auto-approve
🔇 Additional comments (4)
rocketmq-client/src/base/client_config.rs (4)
66-67: LGTM!The new configuration fields
trace_msg_batch_numandmax_page_size_in_get_metadataare appropriately typed and follow the existing naming conventions.
128-140: LGTM!The explicit
.parse::<bool>().unwrap_or(false)pattern is now consistent with other boolean environment variable parsing in this file (e.g.,decode_read_body,vip_channel_enabled). The default values for the new fields are reasonable.
246-537: LGTM!The comprehensive getters and setters are consistently implemented with proper
#[inline]annotations. Theset_namespacemethod correctly updatesnamespace_initializedtotrue(line 286), maintaining state consistency.
593-638: LGTM!The
Displayimplementation correctly includes all fields, including the newly addedtrace_msg_batch_numandmax_page_size_in_get_metadata.
| pub fn reset_client_config(&mut self, other: &ClientConfig) { | ||
| self.namesrv_addr = other.namesrv_addr.clone(); | ||
| self.client_ip = other.client_ip.clone(); | ||
| self.instance_name = other.instance_name.clone(); | ||
| self.client_callback_executor_threads = other.client_callback_executor_threads; | ||
| self.namespace = other.namespace.clone(); | ||
| self.namespace_v2 = other.namespace_v2.clone(); | ||
| self.access_channel = other.access_channel; | ||
| self.poll_name_server_interval = other.poll_name_server_interval; | ||
| self.heartbeat_broker_interval = other.heartbeat_broker_interval; | ||
| self.persist_consumer_offset_interval = other.persist_consumer_offset_interval; | ||
| self.pull_time_delay_millis_when_exception = other.pull_time_delay_millis_when_exception; | ||
| self.unit_mode = other.unit_mode; | ||
| self.unit_name = other.unit_name.clone(); | ||
| self.decode_read_body = other.decode_read_body; | ||
| self.decode_decompress_body = other.decode_decompress_body; | ||
| self.vip_channel_enabled = other.vip_channel_enabled; | ||
| self.use_heartbeat_v2 = other.use_heartbeat_v2; | ||
| self.use_tls = other.use_tls; | ||
| self.socks_proxy_config = other.socks_proxy_config.clone(); | ||
| self.language = other.language; | ||
| self.mq_client_api_timeout = other.mq_client_api_timeout; | ||
| self.detect_timeout = other.detect_timeout; | ||
| self.detect_interval = other.detect_interval; | ||
| self.enable_stream_request_type = other.enable_stream_request_type; | ||
| self.send_latency_enable = other.send_latency_enable; | ||
| self.start_detector_enable = other.start_detector_enable; | ||
| self.enable_heartbeat_channel_event_listener = other.enable_heartbeat_channel_event_listener; | ||
| self.enable_trace = other.enable_trace; | ||
| self.trace_topic = other.trace_topic.clone(); | ||
| self.trace_msg_batch_num = other.trace_msg_batch_num; | ||
| self.max_page_size_in_get_metadata = other.max_page_size_in_get_metadata; | ||
| } |
There was a problem hiding this comment.
namespace_initialized is not reset, which may cause inconsistent state.
The reset_client_config method copies all fields except namespace_initialized. If other.namespace has a value but self.namespace_initialized remains false, subsequent calls to get_namespace() may re-parse and overwrite the copied namespace.
🔎 Suggested fix
self.namespace = other.namespace.clone();
+ self.namespace_initialized
+ .store(other.namespace_initialized.load(Ordering::Acquire), Ordering::Release);
self.namespace_v2 = other.namespace_v2.clone();🤖 Prompt for AI Agents
In rocketmq-client/src/base/client_config.rs around lines 547 to 579,
reset_client_config copies all fields from other but omits
namespace_initialized, which can leave self in an inconsistent state (causing
get_namespace to re-parse/overwrite the copied namespace); update the method to
also assign self.namespace_initialized = other.namespace_initialized so the
initialized flag mirrors the source and prevents redundant re-parsing.
| /// Deprecated: Use with_namespace instead | ||
| #[inline] | ||
| #[deprecated(note = "Use with_namespace for namespace wrapping")] | ||
| pub fn without_namespace(&mut self, resource: &str) -> CheetahString { | ||
| if let Some(namespace) = self.get_namespace().as_deref() { | ||
| NamespaceUtil::without_namespace_with_namespace(resource, namespace).into() | ||
| } else { | ||
| NamespaceUtil::without_namespace(resource).into() | ||
| } | ||
| } |
There was a problem hiding this comment.
Deprecation note is misleading.
The without_namespace method removes the namespace prefix from a resource, while with_namespace adds a namespace prefix. Suggesting with_namespace as a replacement is incorrect—they perform opposite operations. Either provide the correct alternative or clarify the intended migration path.
🤖 Prompt for AI Agents
In rocketmq-client/src/base/client_config.rs around lines 581 to 590, the
deprecation note on the without_namespace method is incorrect (it points to
with_namespace, which does the opposite); update the deprecation annotation and
message to point to the correct replacement(s) — either
NamespaceUtil::without_namespace or
NamespaceUtil::without_namespace_with_namespace depending on the desired
overload — so the note accurately directs callers to the function that removes
namespace prefixes.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5365 +/- ##
==========================================
- Coverage 37.17% 37.10% -0.08%
==========================================
Files 800 800
Lines 108077 108308 +231
==========================================
+ Hits 40182 40187 +5
- Misses 67895 68121 +226 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rocketmq-rust-bot
left a comment
rocketmq-rust-bot
left a comment
There was a problem hiding this comment.
LGTM - All CI checks passed ✅
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #5364
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Deprecated
✏️ Tip: You can customize this high-level summary in your review settings.