[GitHub] [pulsar] michaeljmarshall commented on a change in pull request #12095: [Tiered Storage] Fix read tiered storage ledger data again

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

michaeljmarshall commented on a change in pull request #12095:
URL: https://github.com/apache/pulsar/pull/12095#discussion_r713459149



##########
File path: tiered-storage/jcloud/src/main/java/org/apache/bookkeeper/mledger/offload/jcloud/impl/BlobStoreBackedReadHandleImpl.java
##########
@@ -92,6 +92,9 @@ public LedgerMetadata getLedgerMetadata() {
     @Override
     public CompletableFuture<LedgerEntries> readAsync(long firstEntry, long lastEntry) {
         log.debug("Ledger {}: reading {} - {}", getId(), firstEntry, lastEntry);
+        if (!inputStream.haveDataToRead()) {

Review comment:
       Yes, I agree this is not thread safe. The calling thread for `readAsync` could be any thread. The `executor` is from an Ordered Executor using the ledger id as the mod. Since all updates to the buffer are run by the same `executor`, the current paradigm is thread safe. We should only update buffer state from within a lambda on this class's executor service.

##########
File path: tiered-storage/jcloud/src/main/java/org/apache/bookkeeper/mledger/offload/jcloud/impl/BlobStoreBackedInputStreamImpl.java
##########
@@ -137,6 +137,18 @@ public void seekForward(long position) throws IOException {
         }
     }
 
+    @Override
+    public boolean haveDataToRead() {
+        log.debug("haveDataToRead cursor: {}, objectLen: {}, readableBytes: {}.",
+                cursor, objectLen, this.buffer.readableBytes());

Review comment:
       Nit: what do you think about wrapping this debug statement in a conditional to prevent unnecessary boxing as well as unnecessary computation?




-- 
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