lizhimins opened a new pull request, #11169: URL: https://github.com/apache/rocketmq/pull/11169
### Which Issue(s) This PR Fixes - Fixes #11168 ### Brief Description Six defects in the tiered storage module, found while analysing 24h of production `tiered_store.log` in one region (302M lines, of which 16,902 ERROR). None of them affect durability — the retry path is correct and loses no data — but together they made the most frequent upload failure in production unreadable, and two are latent hazards. **1. `FileSegment#handleCommitException` no longer performs a remote lookup, which fixes the misleading message and an IO-thread hazard at once.** `TieredStoreException` defaults `position` to `-1`, colliding with `GET_FILE_SIZE_ERROR = -1L`. A provider that only sets `position` when it received an HTTP error response leaves it at `-1` for transport failures, so those took the same branch as a genuinely failed HEAD and were logged as `get file size error after commit` — a lookup that never ran. In production, 7,379 such lines in 24h, every one with `expect == commit + content`, i.e. all values local. The same ternary's other side calls `this.getSize()`, a synchronous object-metadata request, from a callback registered via `.exceptionally(...)` with no executor. It therefore runs on whichever thread completed the future — a netty IO thread for a network provider, as the production thread names confirm (`AsyncHttpClient-3-N`). The handler now reconciles only from the length the provider reported on the exception, which costs nothing, and otherwise leaves the input stream in place. `commitPosition` is not advanced on failure, so `needCommit()` stays true and the next `commitAsync` performs the lookup on the dispatcher thread, where that logic already existed. `GET_FILE_SIZE_ERROR` regains a single meaning, so a failed lookup can only be reported by the code that performs it. For a transport failure the number of remote lookups is unchanged: that path never queried the size here either. **2. One log line per commit failure, with a shared vocabulary.** `handleCommitException` logged three differently worded messages with three different field sets, one of which described a *success* (the append landed remotely and only the response was lost). Both commit-failure paths now emit a single line with the same fields and a `result` discriminator: `REMOTE_LANDED`, `RETRY_AFTER_REWIND`, `RETRY_AFTER_RECONCILE` from `handleCommitException`, and `SIZE_LOOKUP_FAILED` from `commitAsync` — which previously named the same quantity `buffer` instead of `content` and omitted `expect` and `remote`. `commit` is now logged as of the append rather than after `correctPosition` overwrote it, so `commit + content == expect` holds on every path and can be read against `remote`. The `!sizeKnown` case is tested first, which also makes `correctPosition(-1)` unreachable by construction rather than by a short-circuit that needs explaining. **3. `FlatAppendFile#destroyExpiredFile` unregisters metadata before deleting the object.** The opposite order leaves a metadata row pointing at a deleted object if the process dies in between. `recover()` reloads it on every restart and every later read of that segment fails with `NoSuchKey`. Production showed 844 such failures in 24h against only 4 distinct object names, concentrated on 3 of 11 affected instances — permanent phantom metadata, not transient errors. The new order fails safe: the worst case is an orphaned object, which costs storage instead of breaking reads. **4. `FileSegment#readAsync` logs a shortened read at WARN instead of DEBUG.** The truncated buffer surfaces much later as a `splitMessageBuffer` failure whose message carries no topic, queueId or offset, so the cause was invisible: 391 `message buffer offset exceeded limit` errors in 24h on one instance that could not be attributed to a queue from the logs. **5. `cacheBusy` compares bytes against bytes.** `fetcherCache.estimatedSize()` counts entries while `memoryMaxSize * 0.8` is a byte count, so the check was effectively always false. It now uses the weighted size, which is what `maximumWeight` bounds via the `SelectBufferResult#getSize` weigher. ### How Did You Test This Change? `mvn -pl tieredstore test` — 127 tests, 0 failures, 0 errors. `mvn -pl tieredstore validate` reports 0 checkstyle violations against `style/rmq_checkstyle.xml`. `FileSegmentTest#handleCommitExceptionTest` already covers all three outcomes and continues to pass unchanged: a provider exception without a position returns false, one carrying `setPosition(size * 2)` equal to the expected position returns true, and a non-`TieredStoreException` cause returns false. I extended the third case with `Mockito.verify(fileSpySegment, Mockito.never()).getSize()`, replacing a `getSize()` stub that the new code no longer reaches, so the no-remote-IO property is guarded by a test rather than only by the commit message. The production figures quoted above come from aggregating 24h of `tiered_store.log` for one region and clustering all 16,902 ERROR lines by root cause. -- 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]
