fuyou001 commented on PR #11073: URL: https://github.com/apache/rocketmq/pull/11073#issuecomment-5580256278
Compatibility consideration: broker member discovery during shutdown The new shutdown guard also covers broker member discovery, which uses `GET_ROUTEINFO_BY_TOPIC` with `SYNC_BROKER_MEMBER_GROUP_PREFIX + brokerName` when `compatibleWithOldNameSrv=true`. There is an existing broker-side error-handling issue on this path: - [`BrokerOuterAPI.getBrokerMemberGroupCompatible()`](https://github.com/apache/rocketmq/blob/086a6b0f9311994dbbb7aea1f0c633b0389a30de/broker/src/main/java/org/apache/rocketmq/broker/out/BrokerOuterAPI.java#L305) turns a `SYSTEM_ERROR` response into an empty `BrokerMemberGroup` instead of throwing. - [`BrokerPreOnlineService.prepareForBrokerOnline()`](https://github.com/apache/rocketmq/blob/086a6b0f9311994dbbb7aea1f0c633b0389a30de/broker/src/main/java/org/apache/rocketmq/broker/BrokerPreOnlineService.java#L248) interprets that empty result as no other brokers being online and calls `startService()`. With `enableSlaveActingMaster=true`, `totalReplicas>1`, and `skipPreOnline=false`, this can bypass the intended HA handshake and, for a returning master, reverse metadata synchronization. **This behavior already exists during startup when `needWaitForService=true` and the NameServer is not ready. It is not a new error-handling defect introduced by this PR.** This change adds a shutdown trigger, including configurations with `needWaitForService=false`, while a request can still receive a response from the shutting-down NameServer. I checked the same broker logic in 5.3.1. A unit-level reproduction using the real member-query and pre-online methods with a mocked `SYSTEM_ERROR` response called `startService()` without calling the HA handshake. Controls with a known peer and a failed handshake, or with `RemotingConnectException`, kept the broker offline. The 26 related NameServer tests passed on the current head. This does not establish actual data corruption in a running cluster. Please consider covering this member-query path in the shutdown regression tests and addressing or explicitly tracking the existing broker-side issue. The safety assertion should be that a failed member lookup keeps the broker isolated and retrying, rather than treating it as a successful empty lookup. Any compatibility solution should account for already deployed older brokers during a NameServer-first rolling upgrade; updating only the new broker implementation would not protect those nodes. Reviewed against `086a6b0f9311994dbbb7aea1f0c633b0389a30de`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
