[ISSUE #9725] Maintain heartbeat between Proxy and Broker#10627
Conversation
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
This PR adds a lifecycle-managed heartbeat service that keeps already-established Proxy-to-Broker remoting channels alive during low-traffic periods, preventing idle-close latency spikes on the next request. The approach is conservative: it only heartbeats over existing active channels, never discovers or connects to new brokers.
Findings
-
[Info]
RemotingClient.getActiveChannelAddresses()— Adding adefaultmethod returningCollections.emptyList()is the right call for backward compatibility. CustomRemotingClientimplementations will not break. TheNettyRemotingClientoverride correctly iterateschannelTablesand checksisOK(). -
[Info]
MQClientAPIExt.getActiveBrokerAddresses()— The NameServer exclusion logic is sound: removes both configured and discovered NameServer addresses from the active channel set. The null/empty guards are thorough. -
[Info]
ProxyBrokerHeartbeatService— The design is well thought out:- No overlap:
AtomicBoolean runningprevents concurrent rounds. - Isolation: Each broker heartbeat is independently try/caught, so one failure does not abort the round.
- Statistics: Per-round success/failure counts are logged, aiding observability.
- One-way: Uses
sendHeartbeatOnewaywhich is appropriate — no response needed for keepalive.
- No overlap:
-
[Info]
ProxyConfig— Defaults (30s interval, 3s timeout, enabled) are sensible. TheinitData()validation (interval > 0, timeout > 0, timeout < interval) prevents misconfiguration at startup. -
[Warning]
ProxyBrokerHeartbeatService.runOnce()— ThesnapshotAddresses()call goes throughClusterServiceManagerwhich may return addresses from a routing cache. If the cache is stale, the heartbeat might skip a recently-added broker. This is acceptable for keepalive (not discovery), but worth noting that newly established channels may take one round before being heartbeated.
Positive Observations
- Conservative scope: Only maintains existing connections, no fan-out or discovery. This minimizes risk.
- Clean lifecycle:
start()/shutdown()withScheduledExecutorServicefollows the same pattern as other Proxy services. - Good test coverage: Tests cover overlap prevention, failure isolation, config validation, null handling, and the NameServer exclusion logic.
- Backward compatible: Default method on
RemotingClientinterface, no existing behavior changed.
Suggestions
- Consider adding a metric/counter for total heartbeat rounds and per-broker failure counts (beyond logging) for integration with monitoring systems.
- The
proxyBrokerHeartbeatTimeoutMillisdefault of 3s seems reasonable, but if the broker is under heavy load, a one-way heartbeat could be delayed. Consider documenting that the timeout should be less than the interval to avoid accumulation.
Overall this is a clean, low-risk improvement to Proxy connection reliability. 👍
Automated review by github-manager-bot
Which Issue(s) This PR Fixes
Brief Description
Proxy opens remoting channels to brokers on demand, but low-traffic channels can remain completely idle long enough to be closed. The next send or consume request then pays the connection re-establishment cost. The root cause is that Proxy has no lifecycle-managed keepalive for its already-established broker channels.
This change:
The default 30-second interval is below the idle close window described in the issue. Existing custom
RemotingClientimplementations remain source/binary compatible through the default empty snapshot implementation.Impact
Low-traffic Proxy deployments retain broker connections that are already in use, avoiding avoidable latency spikes on the next request. NameServer connections are excluded, no broker-wide fan-out is introduced, and the behavior can be disabled or tuned through
ProxyConfig.How Did You Test This Change?
Ran on JDK 8:
mvn -pl proxy -am test -Dtest=ProxyBrokerHeartbeatServiceTest,ProxyBrokerHeartbeatConfigTest,MQClientAPIExtTest,NettyRemotingClientTest -Dsurefire.failIfNoSpecifiedTests=false -Dspotbugs.skip=true -Djacoco.skip=trueResult: 11-module reactor build successful; 46 targeted tests passed with 0 failures, 0 errors, and 0 skipped. Checkstyle reported 0 violations. The reviewed diff contains 10 files and 983 added lines.