[ISSUE #9698] Add scriptable cluster health checks#10628
Conversation
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
This PR adds a scriptable mqadmin clusterHealth command that performs broker and NameServer health checks with reliable exit codes — addressing a real gap for container-based deployments that currently lack a built-in health probe.
Findings
-
[Info]
ClusterHealthChecker.probeAll()— The overall deadline viaScheduledExecutorService.shutdownNow()is a good pattern. One consideration: if a broker probe future is cancelled mid-flight, the underlyinginvokeSyncon the remoting layer may not immediately respect the interrupt. This is acceptable for a health check (best-effort), but worth documenting that the timeout is a soft upper bound. -
[Info]
ClusterHealthChecker.selectTargets()— The deduplication viaidentitystring (cluster + broker + brokerId + address) is correct and handles edge cases like the same address appearing under different broker names. Good defensive null/empty checks throughout. -
[Info]
ClusterHealthReport.toJson()— The JSON output is manually constructed (no Jackson/Gson dependency). This is a reasonable choice for the tools module to avoid pulling in extra dependencies. Just ensure special characters in broker names/addresses are properly escaped if they ever contain quotes or backslashes. -
[Info]
ClusterHealthSubCommand— The exit code contract (0= healthy,1= unhealthy,2= usage error) is clean and follows standard Unix conventions. The--format jsonoption is excellent for automation. -
[Info]
mqhealthcheckshell script — Follows the same pattern as othermq*scripts. Consistent and clean.
Positive Observations
- Well-structured: Clear separation between request, checker, report, and CLI layers.
- Good test coverage: Tests cover concurrency, timeout, malformed metadata, masters-only filtering, and output formatting.
- Backward compatible: Only adds new functionality, no existing APIs changed.
- Concurrency safety: Bounded thread pool, overall deadline, per-broker exception isolation.
Suggestions
- Consider adding a
--verboseflag that includes per-broker latency in the text output for interactive debugging. - The
ClusterHealthRequest.validate()method could benefit from a check thatparallelism > 0(currently only checks>= 1viatimeoutMillis >= 0).
Overall this is a solid addition to the operations tooling. 👍
Automated review by github-manager-bot
Which Issue(s) This PR Fixes
Brief Description
Problem
RocketMQ distributions do not provide a health-check command with reliable process exit codes. Container users currently have to install tools such as nc or nmap and can only test whether a port is open, or build a producer/consumer probe that creates traffic and resources.
Root Cause
The existing mqadmin brokerStatus command is designed for interactive diagnostics: it prints runtime data, does not validate a cluster as a whole, and MQAdminStartup catches command failures without exposing a health-specific exit-code contract. There is also no NameServer-only probe for an empty bootstrap cluster.
Changes
Impact
Container images and orchestration systems can use the bundled command directly in Docker HEALTHCHECK or readiness scripts. Operators can distinguish a live RPC service from an open TCP port, inspect machine-readable details, and check an empty NameServer independently from broker availability. Existing mqadmin commands and behavior are unchanged.
Example uses:
How Did You Test This Change?
JDK 8 (Temurin 8.0.482):
Commands:
mvn -pl tools test -Dtest=ClusterHealthRequestTest,ClusterHealthReportTest,ClusterHealthCheckerTest,ClusterHealthSubCommandTest,MQHealthCheckStartupTest -Dspotbugs.skip=true -Djacoco.skip=true
mvn -pl tools test -Dspotbugs.skip=true -Djacoco.skip=true
mvn -pl tools -am package -DskipTests -Dspotbugs.skip=true -Djacoco.skip=true