[GitHub] [pulsar] eolivelli commented on a diff in pull request #15063: [improve][tiered storage] Reduce cpu usage when offloading the ledger

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

eolivelli commented on code in PR #15063:
URL: https://github.com/apache/pulsar/pull/15063#discussion_r860669252


##########
tiered-storage/jcloud/src/main/java/org/apache/bookkeeper/mledger/offload/jcloud/impl/BlockAwareSegmentInputStreamImpl.java:
##########
@@ -161,6 +211,45 @@ private List<ByteBuf> readNextEntriesFromLedger(long start, long maxNumberEntrie
         }
     }
 
+    @Override
+    public int read(byte[] b, int off, int len) throws IOException {
+        if (b == null) {
+            throw new NullPointerException();
+        } else if (off < 0 || len < 0 || len > b.length - off) {
+            throw new IndexOutOfBoundsException();

Review Comment:
   is it worth to report the values, like IndexOutOfBoundsException("off "+ off +", len="+len+", b.length="+b.length)



##########
tiered-storage/jcloud/src/test/java/org/apache/bookkeeper/mledger/offload/jcloud/impl/BlockAwareSegmentInputStreamTest.java:
##########
@@ -226,7 +236,12 @@ public void testHaveEndPadding() throws Exception {
         // verify read inputStream
         // 1. read header. 128
         byte headerB[] = new byte[DataBlockHeaderImpl.getDataStartOffset()];
-        ByteStreams.readFully(inputStream, headerB);
+        if (useBufferRead) {
+            int ret = inputStream.read(headerB, 0, DataBlockHeaderImpl.getDataStartOffset());

Review Comment:
   do we have a test in which the offset is not 0 ?
   the risk is that is works only with 0 (and we are not handling well the offset)



##########
tiered-storage/jcloud/src/main/java/org/apache/bookkeeper/mledger/offload/jcloud/impl/BlockAwareSegmentInputStreamImpl.java:
##########
@@ -186,6 +275,10 @@ public void close() throws IOException {
             entriesByteBuf.forEach(buf -> buf.release());
             entriesByteBuf.clear();
         }
+        if (paddingBuf.refCnt() != 0) {

Review Comment:
   this is very dangerous.
   we are blindly decreasing the ref count
   we should decrease the ref count only if we incremented it and the same number of times



##########
tiered-storage/jcloud/src/test/java/org/apache/bookkeeper/mledger/offload/jcloud/impl/BlockAwareSegmentInputStreamTest.java:
##########
@@ -256,13 +280,35 @@ public void testHaveEndPadding() throws Exception {
         int left = blockSize - DataBlockHeaderImpl.getDataStartOffset() -  expectedEntryCount * (entrySize + 4 + 8);
         assertEquals(left, 5);
         byte padding[] = new byte[left];
-        inputStream.read(padding);
+        if (useBufferRead) {
+            int ret = 0;
+            int offset = 0;
+            while ((ret = inputStream.read(padding, offset, padding.length - offset)) > 0) {

Review Comment:
   is it possible that this returns 0 ?
   -1 means EOF, but 0 means that there is a dummy read that read nothing



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