poorbarcode commented on code in PR #19783:
URL: https://github.com/apache/pulsar/pull/19783#discussion_r2086170575
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2002,11 +2009,16 @@ CompletableFuture<ReadHandle> getLedgerHandle(long
ledgerId) {
void invalidateReadHandle(long ledgerId) {
CompletableFuture<ReadHandle> rhf = ledgerCache.remove(ledgerId);
if (rhf != null) {
- rhf.thenAccept(ReadHandle::closeAsync)
- .exceptionally(ex -> {
- log.warn("[{}] Failed to close a Ledger ReadHandle:",
name, ex);
- return null;
- });
+ rhf.thenCompose(r -> {
+ if (r instanceof OffloadedLedgerHandle) {
+ log.info("[{}] Closing ledger {} from offload driver {}",
name, ledgerId,
+
config.getLedgerOffloader().getOffloadDriverName());
+ }
+ return r.closeAsync();
+ }).exceptionally(ex -> {
+ log.warn("[{}] Failed to close Ledger ReadHandle {}:", name,
ledgerId, ex);
Review Comment:
Can we add a ledger handle type into this log line, which may be helpful to
find the root cause if we get such error.
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -341,6 +342,10 @@ public boolean isFenced() {
@VisibleForTesting
Map<String, byte[]> createdLedgerCustomMetadata;
+ private long lastEvictOffloadedLedgers;
+ private boolean evictOffloadedLedgersInProgress;
Review Comment:
- The variable `evictOffloadedLedgersInProgress` is for protecting that no
concurrent calls happen to the method `internalEvictOffloadedLedgers
- There is already a lock `synchronized` on the method
`evictOffloadedLedgersInProgress`, I think we can remove the variable
`evictOffloadedLedgersInProgress`.
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedLedgerConfig.java:
##########
@@ -80,6 +80,7 @@ public class ManagedLedgerConfig {
private ManagedLedgerInterceptor managedLedgerInterceptor;
private Map<String, String> properties;
private int inactiveLedgerRollOverTimeMs = 0;
+ private int inactiveOffloadedLedgerEvictionTimeMs = 0;
Review Comment:
Should it be a long value?
##########
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:
- Rather than calling `touch` when exactly executing the reading command,
we'd better call `touch` once more when adding the reading task into the
executor's queue, because the reading task will eventually execute once it is
added into the queue.
- Do we need to add a read-write lock to avoid the following issue?
- The reading task concurrently executes with cleaning a idle offloaded
ledger handle
--
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]
