iloveeyjafjalla opened a new pull request, #11422:
URL: https://github.com/apache/rocketmq/pull/11422
### Which Issue(s) This PR Fixes
- Fixes #11420
### Brief Description
When the append hits the end of a commit log file, `CommitLog` retries on a
fresh file but only uses the retry result to decide whether to call
`onCommitLogAppend`; a failing retry fell through to the unconditional success:
```java
result = mappedFile.appendMessage(msg, this.appendMessageCallback,
putMessageContext);
if (AppendMessageStatus.PUT_OK.equals(result.getStatus())) {
onCommitLogAppend(msg, result, mappedFile);
}
break;
...
PutMessageResult putMessageResult = new
PutMessageResult(PutMessageStatus.PUT_OK, result);
```
A message larger than `mappedFileSizeCommitLog` returns `END_OF_FILE` from
the first append *and* from the retry (a fresh file has no more room than the
file size), so the store answered `PUT_OK` for a message that was never
appended — and since `increaseOffset` is guarded by `PUT_OK` as well, no queue
offset was advanced either. `SendMessageProcessor` turns that status into
`ResponseCode.SUCCESS` and returns a msgId, i.e. the producer receives a
successful send for a dropped message.
This change maps a failed retry the same way the first-append branch already
maps its failures (`UNKNOWN_ERROR` → `UNKNOWN_ERROR`, everything else →
`MESSAGE_ILLEGAL`) and returns it, in both `asyncPutMessage` and
`asyncPutMessages` (the batch path had the identical shape).
### How Did You Test This Change?
Two regression tests, each with a store configured like this repository's
own small-file tests (`mappedFileSizeCommitLog = 1024 * 8`) and a payload
larger than one commit log file but far below `maxMessageSize`:
-
`DefaultMessageStoreTest#testPutMessageLargerThanCommitLogFileIsNotReportedAsSuccess`
(single message, 16 KB body)
-
`BatchPutMessageTest#testPutBatchLargerThanCommitLogFileIsNotReportedAsSuccess`
(batch of four 8 KB messages)
Both assert that the append status is `END_OF_FILE` and that the put status
is not `PUT_OK`.
Results on Temurin JDK 8 (the JDK CI uses), base `a7edeecfb` plus this patch:
- before the fix, both tests fail with `java.lang.AssertionError: Values
should be different. Actual: PUT_OK`
- after the fix: `mvn -B -ntp -pl store test
-Dtest='DefaultMessageStoreTest,AppendCallbackTest,BatchConsumeQueueTest,BatchPutMessageTest'`
— `Tests run: 50, Failures: 0, Errors: 0, Skipped: 0`, `BUILD SUCCESS` (the
four classes that exercise the put path, including the 38
`DefaultMessageStoreTest` cases)
- `mvn -B -ntp -pl store clean package` (checkstyle + spotbugs enabled):
`You have 0 Checkstyle violations.`, `BugInstance size is 0`
Disclosure — the full `store` suite does not complete on this machine, for
three reasons I verified are independent of the patch:
- `HAServerTest#putRequest_MultipleAckAndRequests` (`expected: PUT_OK but
was: FLUSH_SLAVE_TIMEOUT`): time/load sensitive; the same failure reproduces at
the base commit in a separate worktree.
- `BatchConsumeMessageTest` (`java.net.BindException: Address already in
use: bind` from `DefaultHAService$AcceptSocketService.beginAccept`): also
reproduces at the base commit, and no other process on the machine listens in
the 30xxx range the test allocates from.
- After those two are excluded, the run ends in a JVM crash inside the
native RocksDB library (`EXCEPTION_ACCESS_VIOLATION` in `librocksdbjni*.dll`,
thread `Time-limited test`) around the 45th test class in the same JVM; every
class involved passes when run in isolation. The change touches only the commit
log put path and no RocksDB code.
(`skipAfterFailureCount=1` is configured for the module, so any of these
failures would otherwise hide the rest of the suite.)
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]
