lhotari commented on code in PR #19783:
URL: https://github.com/apache/pulsar/pull/19783#discussion_r2116104445
##########
tiered-storage/jcloud/src/main/java/org/apache/bookkeeper/mledger/offload/jcloud/impl/BlobStoreBackedReadHandleImpl.java:
##########
@@ -120,6 +123,7 @@ public CompletableFuture<LedgerEntries> readAsync(long
firstEntry, long lastEntr
}
CompletableFuture<LedgerEntries> promise = new CompletableFuture<>();
executor.execute(() -> {
+ touch();
Review Comment:
> The value can be modified. What do you think is approximately safe to set
this to at least?
@poorbarcode Not many entries get read at once, I believe up to 100 entries.
As long as the value is high enough, it should be sufficient for addressing
the memory leak that currently occurs when these entries don't get removed from
the cache at all.
I understand your request to add a reference count based solution. That
would require a lot more changes to properly avoid any race conditions that
while the eviction loop is running that a ledger gets accessed at that moment
and gets closed. I'd suggest that we'd merge this PR as-is and revisit more
advanced solutions in further PRs if there's really a need to do so. Retries
would handle the case where the ledger handle would get closed in a race
condition so I don't think that it would be a critical problem even without
addressing it.
--
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]
