-
Notifications
You must be signed in to change notification settings - Fork 218
[ISSUE #5527] Update default controller listen port to 60109 #5530
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
🔊@LED-0102 🚀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💥. |
WalkthroughUpdates the default controller listen address port from 9876 to 60109 across configuration files and examples. The changes include the core controller configuration, test helpers, integration examples, and documentation, with no logic or API modifications. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @rocketmq-common/src/common/controller/controller_config.rs:
- Line 244: Update all hardcoded loopback addresses that still use
"127.0.0.1:9876" to the new default "127.0.0.1:60109" so they match the
listen_addr default in controller_config.rs (where listen_addr is set to
"127.0.0.1:60109".parse().unwrap()). Search for the literal "127.0.0.1:9876" in
tests and examples (including the listed test functions,
openraft_integration_test, three_node_cluster example, and the modules using
transport/replica/replicas_info_manager) and replace each occurrence with
"127.0.0.1:60109"; ensure any assertions or expected addresses are updated
accordingly and run tests to verify no remaining hardcoded 9876 usages remain.
🧹 Nitpick comments (1)
CHANGELOG.md (1)
25-25: LGTM! Consider emphasizing the breaking change nature.The CHANGELOG entry correctly documents the port change. However, since changing the default port is a breaking change that may affect existing deployments, consider making this more explicit, for example:
-- **chore(controller):** Update default controller listen address to use port 60109 ([#5527](https://github.com/mxsm/rocketmq-rust/issues/5527)) +- **chore(controller):** Update default controller listen address to use port 60109 (BREAKING CHANGE: affects new deployments using default configuration) ([#5527](https://github.com/mxsm/rocketmq-rust/issues/5527))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.mdrocketmq-common/src/common/controller/controller_config.rsrocketmq-controller/examples/controller_metrics_example.rsrocketmq-controller/examples/single_node.rs
🧰 Additional context used
🧬 Code graph analysis (1)
rocketmq-controller/examples/controller_metrics_example.rs (1)
rocketmq-common/src/common/controller/controller_config.rs (2)
new(257-259)new_node(415-417)
⏰ 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). (1)
- GitHub Check: auto-approve
🔇 Additional comments (3)
rocketmq-common/src/common/controller/controller_config.rs (1)
421-421: LGTM! Test helper updated consistently.The test configuration helper correctly uses the new default port 60109, maintaining consistency with the main default implementation.
rocketmq-controller/examples/controller_metrics_example.rs (1)
40-40: LGTM! Example updated to reflect new default port.The example correctly demonstrates the new default controller port 60109, consistent with the updated ControllerConfig defaults.
rocketmq-controller/examples/single_node.rs (1)
43-43: LGTM! Example updated consistently.The single-node example correctly uses the new default port 60109, maintaining consistency across all configuration files and examples.
| config_black_list: "configBlackList;configStorePath".to_string(), | ||
| node_id: 1, | ||
| listen_addr: "127.0.0.1:9876".parse().unwrap(), | ||
| listen_addr: "127.0.0.1:60109".parse().unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for remaining references to port 9876
echo "=== Searching for port 9876 references ==="
rg -n "9876" --type rust -C 2
echo -e "\n=== Searching for old listen address pattern ==="
rg -n "127\.0\.0\.1:9876" -C 2
echo -e "\n=== Searching for port 60109 references to confirm updates ==="
rg -n "60109" --type rust -C 2Repository: mxsm/rocketmq-rust
Length of output: 50377
🏁 Script executed:
# Read the controller_config.rs file around line 244
cat -n rocketmq-common/src/common/controller/controller_config.rs | sed -n '235,255p'
# Find all config files with default port/listen_addr settings
echo -e "\n=== Searching for listen_addr defaults in config files ==="
rg -n "listen_addr.*:" --type rust rocketmq-common/src/common/controller/ -C 1
# Check if there are other default listen_addr patterns in controller configs
echo -e "\n=== Checking for other port defaults in controller modules ==="
rg -n "default.*addr\|listen.*default\|port.*default" --type rust rocketmq-common/src/common/controller/ -iRepository: mxsm/rocketmq-rust
Length of output: 2538
🏁 Script executed:
# Search specifically for the address pattern 127.0.0.1:9876
echo "=== Direct port 9876 address references ==="
rg "127\.0\.0\.1:9876" --type rust
# Check if those references are in test code
echo -e "\n=== Checking context of consumer_connection.rs reference ==="
rg -B 3 -A 3 "127\.0\.0\.1:9876" rocketmq-remoting/src/protocol/body/consumer_connection.rs
echo -e "\n=== Checking context of get_meta_data_response_header.rs references ==="
rg -B 3 -A 3 "127\.0\.0\.1:9876" rocketmq-remoting/src/protocol/header/get_meta_data_response_header.rsRepository: mxsm/rocketmq-rust
Length of output: 14472
🏁 Script executed:
# Check the controller test files
echo "=== Checking metrics_quick_reference_test.rs ==="
cat -n rocketmq-controller/tests/metrics_quick_reference_test.rs | head -30
echo -e "\n=== Checking three_node_cluster.rs example ==="
cat -n rocketmq-controller/examples/three_node_cluster.rs | head -50 | tail -30Repository: mxsm/rocketmq-rust
Length of output: 2552
🏁 Script executed:
# Check more of the three_node_cluster example to see the node configurations
echo "=== Checking three_node_cluster.rs node address configuration ==="
rg -A 5 "BTreeMap" rocketmq-controller/examples/three_node_cluster.rs | head -40
# Count how many test/example files still hardcode 9876 for controller
echo -e "\n=== Files in rocketmq-controller using 127.0.0.1:9876 ==="
rg -l "127\.0\.0\.1:9876" rocketmq-controller/Repository: mxsm/rocketmq-rust
Length of output: 843
🏁 Script executed:
# Check the context in the controller test files - are they intentional test mocks or should they use the new default?
echo "=== Context from metrics_quick_reference_test.rs (all lines with 9876) ==="
rg -B 2 -A 2 "127\.0\.0\.1:9876" rocketmq-controller/tests/metrics_quick_reference_test.rs | head -40
echo -e "\n=== Context from openraft_integration_test.rs (first occurrence) ==="
rg -B 3 -A 3 "127\.0\.0\.1:9876" rocketmq-controller/tests/openraft_integration_test.rs | head -15Repository: mxsm/rocketmq-rust
Length of output: 2100
Update controller test and example files to use port 60109 for consistency with the new default.
While the default listen_addr in controller_config.rs has been correctly updated to port 60109, multiple test and example files in rocketmq-controller still hardcode "127.0.0.1:9876":
- rocketmq-controller/tests/metrics_quick_reference_test.rs (9 test functions)
- rocketmq-controller/tests/openraft_integration_test.rs
- rocketmq-controller/examples/three_node_cluster.rs
- rocketmq-controller/src/raft/transport.rs
- rocketmq-controller/src/metadata/replica.rs
- rocketmq-controller/src/manager/replicas_info_manager.rs
These should be updated to "127.0.0.1:60109" to maintain consistency with the new default port.
🤖 Prompt for AI Agents
In @rocketmq-common/src/common/controller/controller_config.rs at line 244,
Update all hardcoded loopback addresses that still use "127.0.0.1:9876" to the
new default "127.0.0.1:60109" so they match the listen_addr default in
controller_config.rs (where listen_addr is set to
"127.0.0.1:60109".parse().unwrap()). Search for the literal "127.0.0.1:9876" in
tests and examples (including the listed test functions,
openraft_integration_test, three_node_cluster example, and the modules using
transport/replica/replicas_info_manager) and replace each occurrence with
"127.0.0.1:60109"; ensure any assertions or expected addresses are updated
accordingly and run tests to verify no remaining hardcoded 9876 usages remain.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5530 +/- ##
==========================================
- Coverage 38.41% 38.41% -0.01%
==========================================
Files 815 815
Lines 110512 110512
==========================================
- Hits 42452 42450 -2
- Misses 68060 68062 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rocketmq-rust-bot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - All CI checks passed ✅
mxsm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LED-0102 Thanks for your contribution, LGTM
Which Issue(s) This PR Fixes(Closes)
Fixes #5527
Brief Description
This PR updates the default controller listen address to use port
60109instead of the previously hardcoded port.The change has been applied consistently across:
ControllerConfigdefault configurationControllerConfig::new_nodePlease let me know if updating these locations is sufficient, or if there are additional places where this port should be updated.
How Did You Test This Change?
cargo buildcargo test --all-features --workspaceSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.