Conversation
…related hazards Found while analysing 24h of production tiered-store logs in cn-hangzhou (302M lines, of which 16,902 ERROR). A transient upload failure was reported as "get file size error after commit" although no size lookup ever took place. TieredStoreException defaults its position to -1, which collides with GET_FILE_SIZE_ERROR, so a provider-reported failure and a failed lookup shared one branch and one message. handleCommitException now performs no remote lookup at all: it reconciles from the length the provider reported on the exception, and otherwise leaves the input stream in place for the next commitAsync, which already did that lookup on the dispatcher thread. This matters because the handler is registered via exceptionally with no executor, so it runs on a netty IO thread for a network provider. GET_FILE_SIZE_ERROR keeps its single meaning, and a failed lookup can only be reported by the code that performs it. The same method logged three differently worded messages with three different field sets, one of which described a success. Both commit failure paths now emit one line with a shared field set and a result discriminator: REMOTE_LANDED, RETRY_AFTER_REWIND and RETRY_AFTER_RECONCILE from handleCommitException, SIZE_LOOKUP_FAILED from commitAsync, which previously called the same quantity "buffer" instead of "content" and omitted expect and remote. The commit position is logged as of the append rather than after correctPosition overwrote it, so commit, content and expect stay mutually consistent. FlatAppendFile#destroyExpiredFile deleted the remote object before unregistering its metadata. A crash in between leaves a metadata row pointing at a deleted object, reloaded on every restart, after which every read of that segment fails with NoSuchKey. Reverse the order so the worst case becomes an orphaned object. FileSegment#readAsync shortened a read to the committed length and logged it at DEBUG. The truncated buffer surfaces later as a splitMessageBuffer failure whose own message carries no topic, queueId or offset, so the cause was invisible in production. Raise it to WARN. MessageStoreFetcherImpl compared fetcherCache.estimatedSize(), a count of entries, against memoryMaxSize * 0.8, a number of bytes, so cacheBusy was always false. Compare the weighted size, which is what maximumWeight bounds.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #11169 +/- ##
=============================================
- Coverage 49.52% 49.42% -0.11%
+ Complexity 14293 14271 -22
=============================================
Files 1390 1390
Lines 103129 103174 +45
Branches 13485 13490 +5
=============================================
- Hits 51075 50989 -86
- Misses 45902 46012 +110
- Partials 6152 6173 +21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
This PR fixes six defects in the tiered storage module, all identified through production log analysis (302M lines, 16,902 ERRORs in 24h). The changes are well-motivated, correctly scoped, and the test coverage properly validates the key behavioral change (no synchronous getSize() call on the exception handler thread).
The most critical fix is defect #1 — removing the synchronous remote metadata lookup from the exceptionally callback, which was running on netty IO threads and blocking them. Deferring reconciliation to the next commitAsync() on the dispatcher thread is the correct approach.
The crash-safety fix in FlatAppendFile#destroyExpiredFile (metadata removal before object deletion) is also important — the previous ordering left metadata pointing at deleted objects if the process died between the two operations.
Overall, this is a high-quality PR with exceptional documentation. Each fix is backed by production evidence, and the unified logging format will make future debugging significantly easier.
Findings
- [Info] FlatAppendFile.java:295 — Orphaned object cleanup after metadata deletion may need a background sweep
- [Info] MessageStoreFetcherImpl.java:464 — Cache weight fix is correct; note about unbounded cache behavior in tests
Suggestions
- Consider adding a background sweep for orphaned objects in
destroyExpiredFileto handle the case wheredestroyFile()throws after metadata is already deleted - The unified logging format (
REMOTE_LANDED,RETRY_AFTER_REWIND,RETRY_AFTER_RECONCILE,SIZE_LOOKUP_FAILED) is excellent — consider documenting these result codes in a developer guide or README
Automated review by RockteMQ-AI
| long baseOffset = fileSegment.getBaseOffset(); | ||
| fileSegmentTable.remove(0); | ||
| metadataStore.deleteFileSegment(filePath, fileType, baseOffset); | ||
| fileSegment.destroyFile(); |
There was a problem hiding this comment.
[Info] The ordering change (metadata removal before object deletion) is correct for crash safety — an orphaned object is indeed harmless while orphaned metadata causes NoSuchKey on every subsequent read.
One consideration: if metadataStore.deleteFileSegment() succeeds but fileSegment.destroyFile() throws, the warn log fires but the segment is already unregistered from fileSegmentTable. This means the orphaned object will never be retried for cleanup. Consider whether a background sweep or a retry loop for destroyFile() would be worthwhile for production hygiene, though this is not a correctness issue.
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Six defects in the tiered storage module, all found from production log analysis (302M lines, 16,902 ERRORs). None affect durability — the retry path is correct — but they made the most frequent upload failure unreadable and two are latent hazards. Well-motivated, production-tested fixes.
Observations
- Cache weight fix (MessageStoreFetcherImpl): Correct — the cache is bounded by
maximumWeight(bytes viaSelectBufferResult#getSizeweigher), soestimatedSize()(entry count) was the wrong metric. UsingweightedSize()from the eviction policy is the right fix. handleCommitExceptionno longer does remote lookup: Significant improvement — removing a network call from the exception handler eliminates a potential cascading failure. Returningfalsewhen size is unknown and letting the next commit reconcile is the right approach.FILE_NOT_FOUNDhandling (IndexStoreFile): Correct — objects can be deleted while reads are in flight. Downgrading to INFO and returning empty results avoids log noise. ThehasErrorCodeutility walks the cause chain with cycle detection (cause != cause.getCause()).readAsynclog upgrade: Debug → warn when request exceeds commit position is appropriate for production observability.- Tests: New tests for cache weight,
hasErrorCode, and the updated commit exception test that verifiesgetSizeis NOT called on the exception path. Good coverage.
LGTM.
Automated review by github-manager-bot
3 participants
Which Issue(s) This PR Fixes
Brief Description
Six defects in the tiered storage module, found while analysing 24h of production
tiered_store.login 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#handleCommitExceptionno longer performs a remote lookup, which fixes the misleading message and an IO-thread hazard at once.TieredStoreExceptiondefaultspositionto-1, colliding withGET_FILE_SIZE_ERROR = -1L. A provider that only setspositionwhen it received an HTTP error response leaves it at-1for transport failures, so those took the same branch as a genuinely failed HEAD and were logged asget file size error after commit— a lookup that never ran. In production, 7,379 such lines in 24h, every one withexpect == 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.
commitPositionis not advanced on failure, soneedCommit()stays true and the nextcommitAsyncperforms the lookup on the dispatcher thread, where that logic already existed.GET_FILE_SIZE_ERRORregains 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.
handleCommitExceptionlogged 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 aresultdiscriminator:REMOTE_LANDED,RETRY_AFTER_REWIND,RETRY_AFTER_RECONCILEfromhandleCommitException, andSIZE_LOOKUP_FAILEDfromcommitAsync— which previously named the same quantitybufferinstead ofcontentand omittedexpectandremote.commitis now logged as of the append rather than aftercorrectPositionoverwrote it, socommit + content == expectholds on every path and can be read againstremote.The
!sizeKnowncase is tested first, which also makescorrectPosition(-1)unreachable by construction rather than by a short-circuit that needs explaining.3.
FlatAppendFile#destroyExpiredFileunregisters 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 withNoSuchKey. 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#readAsynclogs a shortened read at WARN instead of DEBUG.The truncated buffer surfaces much later as a
splitMessageBufferfailure whose message carries no topic, queueId or offset, so the cause was invisible: 391message buffer offset exceeded limiterrors in 24h on one instance that could not be attributed to a queue from the logs.5.
cacheBusycompares bytes against bytes.fetcherCache.estimatedSize()counts entries whilememoryMaxSize * 0.8is a byte count, so the check was effectively always false. It now uses the weighted size, which is whatmaximumWeightbounds via theSelectBufferResult#getSizeweigher.How Did You Test This Change?
mvn -pl tieredstore test— 127 tests, 0 failures, 0 errors.mvn -pl tieredstore validatereports 0 checkstyle violations againststyle/rmq_checkstyle.xml.FileSegmentTest#handleCommitExceptionTestalready covers all three outcomes and continues to pass unchanged: a provider exception without a position returns false, one carryingsetPosition(size * 2)equal to the expected position returns true, and a non-TieredStoreExceptioncause returns false. I extended the third case withMockito.verify(fileSpySegment, Mockito.never()).getSize(), replacing agetSize()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.logfor one region and clustering all 16,902 ERROR lines by root cause.