Skip to content

Conversation

@onenewcode
Copy link
Contributor

@onenewcode onenewcode commented Jan 8, 2026

Which Issue(s) This PR Fixes(Closes)

Fixes #5544

Brief Description

Implement the handle_get_controller_config method in ControllerRequestProcessor to handle queries for controller configuration. This feature allows administrators and monitoring tools to retrieve the current controller configuration settings.

How Did You Test This Change?

Add some tests

Summary by CodeRabbit

  • New Features

    • Controller configuration can now be retrieved and exported in a standard camelCase properties format, including raft peer listings.
    • Storage backend types are represented consistently as readable strings in the export.
    • Remote requests for controller config now return the serialized properties payload.
  • Tests

    • Added tests validating output structure, line formatting, custom settings, and raft-peer serialization.

✏️ Tip: You can customize this high-level summary in your review settings.

@rocketmq-rust-bot
Copy link
Collaborator

🔊@onenewcode 🚀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 rocketmq-rust-bot added the AI review first Ai review pr first label Jan 8, 2026
@rocketmq-rust-robot rocketmq-rust-robot added the feature🚀 Suggest an idea for this project. label Jan 8, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 8, 2026

Walkthrough

Added Display for StorageBackendType and a public ControllerConfig::to_properties_string() that serializes config into camelCase key=value properties. Implemented ControllerRequestProcessor::handle_get_controller_config to return the serialized properties as a RemotingCommand response. Tests added for the serialization output.

Changes

Cohort / File(s) Summary
Configuration Serialization
rocketmq-common/src/common/controller/controller_config.rs
Added impl fmt::Display for StorageBackendType mapping variants to "rocks_db", "file", "memory". Added pub fn to_properties_string(&self) -> String producing multi-line camelCase key=value properties; serializes raft_peers as a semicolon-delimited list of id-addr entries under raftPeers. Added unit tests for output structure, per-line = constraint, custom config fields, and raft_peers formatting.
Request Handler Implementation
rocketmq-controller/src/processor/controller_request_processor.rs
Implemented handle_get_controller_config to fetch controller_config() from controller_manager, call to_properties_string(), set the resulting bytes as the RemotingCommand response body, and return the response.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant ControllerRequestProcessor as Processor
    participant ControllerManager as Manager
    participant RemotingCommand as Response

    Client->>Processor: GetControllerConfig request
    Processor->>Manager: controller_config()
    Manager-->>Processor: ControllerConfig
    Processor->>Processor: ControllerConfig::to_properties_string()
    Processor->>Response: create_response_command()
    Processor->>Response: set_body(properties bytes)
    Processor-->>Client: RemotingCommand (200, body)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A hop, a nibble, configs now sing,
Keys in camelCase on a tidy string,
Rocks, File, Memory — each name on show,
A handler fetches, returns what you know,
Hooray! Little rabbit runs off to grow 🌿

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main feature being implemented: the GetControllerConfig request handler.
Linked Issues check ✅ Passed All coding requirements from issue #5544 are met: to_properties_string method added, handle_get_controller_config implemented, configuration serialization completed, and proper response formatting implemented.
Out of Scope Changes check ✅ Passed All changes directly support implementing the GetControllerConfig handler; no unrelated modifications are present in the changeset.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 971d256 and c26fa2c.

📒 Files selected for processing (2)
  • rocketmq-common/src/common/controller/controller_config.rs
  • rocketmq-controller/src/processor/controller_request_processor.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • rocketmq-common/src/common/controller/controller_config.rs
⏰ 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). (2)
  • GitHub Check: Check (fmt + clippy)
  • GitHub Check: auto-approve
🔇 Additional comments (1)
rocketmq-controller/src/processor/controller_request_processor.rs (1)

448-452: Implementation is correct and safe. The handle_get_controller_config function properly retrieves the controller configuration, serializes it to the correct properties format, and returns it as the response body. The ControllerConfig struct contains no sensitive fields, so there is no security concern with exposing all fields via this endpoint.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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:
- Around line 698-699: The test assertion line in controller_config.rs (the
assert! checking result.contains("raftPeers=1-127.0.0.1:9877;2-127.0.0.1:9878"))
is misformatted and fails `cargo fmt --check`; run `cargo fmt` from the
repository root to reformat the file so the assert and surrounding code conform
to rustfmt rules, then re-run `cargo fmt --check` or `cargo test` to confirm the
CI formatting error is resolved.
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 44bbd1c and 971d256.

📒 Files selected for processing (2)
  • rocketmq-common/src/common/controller/controller_config.rs
  • rocketmq-controller/src/processor/controller_request_processor.rs
🧰 Additional context used
🧬 Code graph analysis (2)
rocketmq-controller/src/processor/controller_request_processor.rs (2)
rocketmq-controller/src/controller/controller_manager.rs (1)
  • controller_config (596-598)
rocketmq-remoting/src/protocol/remoting_command.rs (1)
  • create_response_command (194-198)
rocketmq-common/src/common/controller/controller_config.rs (1)
rocketmq-common/src/common/namesrv/namesrv_config.rs (1)
  • rocketmq_home (31-33)
🪛 GitHub Actions: RocketMQ Rust CI
rocketmq-common/src/common/controller/controller_config.rs

[error] 698-699: cargo fmt --check failed. Code formatting issue detected (diff shown in log). Run 'cargo fmt' to fix formatting.

⏰ 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 (4)
rocketmq-common/src/common/controller/controller_config.rs (3)

57-65: LGTM! Clean Display implementation.

The Display trait implementation for StorageBackendType provides clear string representations that are used effectively in the properties serialization.


468-562: Well-implemented configuration serialization.

The to_properties_string() method provides a clean, comprehensive serialization of all controller configuration fields in camelCase properties format. The raft peers formatting (line 546: "id-addr") is internally consistent with the tests and provides a reasonable, parseable format.


638-700: Excellent test coverage for properties serialization.

The test suite comprehensively validates the properties string generation including format validation, custom configurations, and raft peers serialization. The tests ensure each line contains exactly one '=' character and verify that custom values are correctly reflected in the output.

rocketmq-controller/src/processor/controller_request_processor.rs (1)

448-453: Perfect implementation aligned with PR objectives.

The handle_get_controller_config handler correctly implements the requirements:

  • Retrieves current controller configuration
  • Serializes it to properties format using the new to_properties_string() method
  • Returns a successful response with the configuration in the body

The implementation is lightweight, read-only, and can be served from any controller node without requiring consensus, exactly as specified in the PR objectives.

@onenewcode onenewcode force-pushed the GetControllerConfig branch from 971d256 to c26fa2c Compare January 8, 2026 01:24
@codecov
Copy link

codecov bot commented Jan 8, 2026

Codecov Report

❌ Patch coverage is 95.68966% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.56%. Comparing base (a6f6cb2) to head (c26fa2c).
⚠️ Report is 21 commits behind head on main.

Files with missing lines Patch % Lines
...ller/src/processor/controller_request_processor.rs 0.00% 4 Missing ⚠️
...-common/src/common/controller/controller_config.rs 99.10% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5554      +/-   ##
==========================================
+ Coverage   38.43%   38.56%   +0.12%     
==========================================
  Files         815      815              
  Lines      110551   110724     +173     
==========================================
+ Hits        42492    42696     +204     
+ Misses      68059    68028      -31     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Collaborator

@rocketmq-rust-bot rocketmq-rust-bot left a 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 ✅

Copy link
Owner

@mxsm mxsm left a comment

Choose a reason for hiding this comment

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

LGTM

@mxsm mxsm merged commit 40fc31e into mxsm:main Jan 9, 2026
10 of 13 checks passed
@rocketmq-rust-bot rocketmq-rust-bot added approved PR has approved and removed ready to review waiting-review waiting review this PR labels Jan 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI review first Ai review pr first approved PR has approved auto merge Difficulty level/Hard Hard ISSUE feature🚀 Suggest an idea for this project.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature🚀] Implement GetControllerConfig Request Handler

4 participants