RockteMQ-AI commented on code in PR #11169:
URL: https://github.com/apache/rocketmq/pull/11169#discussion_r4016213556
##########
tieredstore/src/main/java/org/apache/rocketmq/tieredstore/core/MessageStoreFetcherImpl.java:
##########
@@ -459,7 +459,12 @@ public CompletableFuture<GetMessageResult> getMessageAsync(
return CompletableFuture.completedFuture(result);
}
- boolean cacheBusy = fetcherCache.estimatedSize() > memoryMaxSize * 0.8;
+ // The cache is bounded by maximumWeight (bytes, via the
SelectBufferResult#getSize weigher),
+ // so compare against weightedSize() rather than estimatedSize(),
which counts entries.
+ long cacheWeight = fetcherCache.policy().eviction()
Review Comment:
**[Info]** Good catch — `estimatedSize()` counts entries while
`memoryMaxSize` is in bytes, so the old comparison was dimensionally incorrect.
Using `weightedSize()` from the eviction policy is the right fix.
The chained `.map(eviction ->
eviction.weightedSize().orElse(0L)).orElse(0L)` is safe, but worth noting: if
the cache has no eviction policy configured (e.g., in a test setup),
`cacheWeight` will be 0 and `cacheBusy` will always be false, effectively
disabling the backpressure. This matches the intent (unbounded cache = never
busy), but a comment or assertion in test configs could prevent confusion.
##########
tieredstore/src/main/java/org/apache/rocketmq/tieredstore/file/FlatAppendFile.java:
##########
@@ -286,10 +286,16 @@ public void destroyExpiredFile(long expireTimestamp) {
break;
}
+ // Unregister the metadata before deleting the object. The
opposite order leaves a
+ // metadata row pointing at a deleted object if the process
dies in between, and every
+ // later read of that segment then fails with NoSuchKey. An
orphaned object is harmless.
+ long baseOffset = fileSegment.getBaseOffset();
+ fileSegmentTable.remove(0);
+ metadataStore.deleteFileSegment(filePath, fileType,
baseOffset);
fileSegment.destroyFile();
Review 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.
--
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]
