[GitHub] [pulsar] hangc0276 commented on a diff in pull request #16417: Shared subscription: improvement with offloaded ledgers

[GitBox <gi...@apache.org>]

hangc0276 commented on code in PR #16417:
URL: https://github.com/apache/pulsar/pull/16417#discussion_r915986946


##########
tiered-storage/jcloud/src/main/java/org/apache/bookkeeper/mledger/offload/jcloud/impl/BlobStoreBackedReadHandleImpl.java:
##########
@@ -55,6 +57,8 @@ public class BlobStoreBackedReadHandleImpl implements ReadHandle {
     private final BackedInputStream inputStream;
     private final DataInputStream dataStream;
     private final ExecutorService executor;
+    // this Map is accessed only by one thread
+    private final Map<Long, Long> entryOffsets = new TreeMap<>();

Review Comment:
   The map won't expire any items until the `ReadHandle` is closed. It may cost a lot of memory. Could we introduce an expiration policy?



##########
tiered-storage/jcloud/src/main/java/org/apache/bookkeeper/mledger/offload/jcloud/impl/BlobStoreBackedReadHandleImpl.java:
##########
@@ -160,7 +170,14 @@ public CompletableFuture<LedgerEntries> readAsync(long firstEntry, long lastEntr
                         // the read method, we should allow to seek to the right position and the entry id should
                         // never over to the last entry again.
                         if (!seeked) {
-                            inputStream.seek(index.getIndexEntryForEntry(nextExpectedId).getDataOffset());
+                            Long knownOffset = entryOffsets.get(nextExpectedId);

Review Comment:
   Maybe we can a function to package this logic and all  the `index.getIndexEntryForEntry(nextExpectedId).getDataOffset()` call this function.
   
   Other seek places also need to check the index cache first.



##########
tiered-storage/jcloud/src/test/java/org/apache/bookkeeper/mledger/offload/jcloud/BlobStoreBackedInputStreamTest.java:
##########
@@ -18,6 +18,7 @@
  */
 package org.apache.bookkeeper.mledger.offload.jcloud;
 
+import static junit.framework.TestCase.assertEquals;

Review Comment:
   Use testng instead of junit?



-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org