iloveeyjafjalla opened a new pull request, #11417:
URL: https://github.com/apache/rocketmq/pull/11417
### Which Issue(s) This PR Fixes
- Fixes #11412
### Brief Description
A `BatchConsumeQueue` that has no mapped file yet threw instead of reporting
“nothing in the queue”, which every sibling implementation answers with `null`
units and a `-1` last offset:
```java
public CqUnit getEarliestUnit() {
return get(minOffsetInQueue); // -1 for a fresh queue ->
getMinMsgOffset(null, …) -> NPE
}
public long getLastOffset() {
CqUnit latestUnit = getLatestUnit(); // null would be dereferenced
return latestUnit.getPos() + latestUnit.getSize();
}
```
The NPE originates in `getMinMsgOffset`, the only one of the two helpers
that does not expect a null file — `getMaxMsgOffset` already guards `mappedFile
== null` (`BatchConsumeQueue.java:656-657`).
This change:
1. `getMinMsgOffset`: treats a null mapped file as “no data”, exactly like
`getMaxMsgOffset` (`:635-636`);
2. `getEarliestUnit()` / `getLatestUnit()`: return `null` when the queue has
no offset to read (`minOffsetInQueue >= 0` / `maxOffsetInQueue > 0`), the way
`RocksDBConsumeQueue.getLatestUnit()` guards its offsets
(`RocksDBConsumeQueue.java:362-372`);
3. `getLastOffset()`: return `-1` when there is no latest unit, the way
`ConsumeQueue.getLastOffset()` returns `-1` without a mapped file
(`ConsumeQueue.java:501-511`).
The caller already expects that sentinel: `ConsumeQueueStore.cleanExpired()`
has a dedicated branch for `-1` (`“maybe ConsumeQueue was created just now”`,
`ConsumeQueueStore.java:593-600`), and
`DefaultMessageStore.getEarliestMessageTime()` treats a null unit as “no time”
(`getStoreTime(null)` returns `-1`). Before this change, `mqadmin
cleanExpiredConsumerQueue` aborted with `SYSTEM_ERROR` on any broker that hosts
a batch topic with an empty queue, and `earliestMsgStoreTime` /
`queryConsumeTimeSpan` failed on that queue.
### How Did You Test This Change?
New test
`BatchConsumeQueueTest#testEmptyBatchConsumeQueueReportsNoUnitAndMinusOneLastOffset`,
built with the existing harness (`createBatchConsume(null)` + `load()`, no
entries):
- before the fix: `java.lang.NullPointerException at
BatchConsumeQueue.getMinMsgOffset(BatchConsumeQueue.java:636)` ←
`getBatchMsgIndexBuffer:683` ← `iterateFrom:328` ← `get:342` ←
`getEarliestUnit:365`
- after the fix: `BatchConsumeQueueTest` — `Tests run: 6, Failures: 0,
Errors: 0`; the test asserts `getEarliestUnit() == null`, `getLatestUnit() ==
null`, `getLastOffset() == -1` and a `-1` earliest store time
Results on Temurin JDK 8 (the JDK CI uses), base `a7edeecfb` plus this patch:
- `mvn -B -ntp -pl store clean package` (checkstyle + spotbugs enabled):
`You have 0 Checkstyle violations.`, `BugInstance size is 0`.
- One unrelated test fails in that module on this machine:
`HAServerTest#putRequest_MultipleAckAndRequests` (`expected: PUT_OK but was:
FLUSH_SLAVE_TIMEOUT`). It is pre-existing and unrelated to this change — I
checked the base commit in a separate worktree (`git worktree add …
upstream/develop`) and it fails there with the same assertion, so I re-ran the
module with that single test excluded to keep the rest of the suite meaningful.
Transparency note on the local environment, because it shaped the run above:
an earlier locally installed `rocketmq-common` jar in this machine had been
built from stale `target/classes` produced by a newer JDK
(`ByteBuffer.flip:()Ljava/nio/ByteBuffer;` in `MessageExt`), which made several
store tests fail with `NoSuchMethodError` on JDK 8 before any assertion ran. I
rebuilt every module with `mvn clean install -DskipTests` on JDK 8, after which
those tests pass (`javap -v` confirms the JDK 8 binding). The failure mode is
environmental and CI is not affected.
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]
