iloveeyjafjalla opened a new pull request, #11368:
URL: https://github.com/apache/rocketmq/pull/11368
### Which Issue(s) This PR Fixes
- Fixes #11367
### Brief Description
`ResetGroupOffset` answered `OK` after resetting only the brokers that
happened to answer.
The reset fanned out through the shared best-effort helper, which keeps the
successful answers and drops the failed hops
(only a fan-out where *every* broker failed is propagated):
```java
queryAllBrokers(brokerAddrs, (addr, timeout) ->
admin().resetOffset(addr, topic, group, resetTimestamp, true,
timeout))
.whenComplete((ignored, throwable) -> { ... ok() ... });
```
Tolerating a partially available cluster is documented for the reads ("a
partially available cluster should still return the
data it has"), but for this mutation it means the caller cannot tell a full
reset from a partial one: the queues on the
broker that did not answer keep consuming from the old offsets (duplicate or
skipped messages) while the RPC reports success.
The change spells the failure policy out in the fan-out helper
(`requireAllBrokers`): a mutation fails the call when any hop
failed and the status message names the brokers that were not reset, while
the read paths keep their best-effort semantics.
`resetGroupOffset` is the only mutation that fans out, so it is the only
caller that passes the flag.
### How Did You Test This Change?
New test
`ProxyAdminGrpcServiceTest#resetGroupOffsetReportsBrokersThatWereNotResetTest`:
two brokers, one applies the reset,
the other fails; the RPC must not answer `OK` and the status must name the
broker that was not reset.
Results on Temurin JDK 8 (same JDK as CI), base `9ea2ccdc2` plus this patch:
- before the fix: `expected:<INTERNAL_SERVER_ERROR> but was:<OK>` (the reset
that reached only one of the two brokers was reported as a success)
- after the fix: `ProxyAdminGrpcServiceTest — Tests run: 61, Failures: 0,
Errors: 0, Skipped: 0`
- `mvn -B -ntp -pl proxy package` with checkstyle + spotbugs enabled: `BUILD
SUCCESS`, spotbugs `BugInstance size is 0`
The existing fan-out tests still pass, including
`resetGroupOffsetPropagatesBrokerFailureTest` (single broker failing keeps
propagating the broker's own error) and the read-path tests that rely on
partial tolerance.
This change was prepared with AI assistance (analysis, patch and test
drafting); the diff and every result above were
reproduced locally by running the module's tests against the base commit and
the patched tree.
--
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]
