You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by "StevenLuMT (via GitHub)" <gi...@apache.org> on 2023/03/28 09:11:28 UTC

[GitHub] [bookkeeper] StevenLuMT opened a new pull request, #3895: [feature] [server] add dbStorage_readAheadCacheBatchBytesSize properties when read ahead entries

StevenLuMT opened a new pull request, #3895:
URL: https://github.com/apache/bookkeeper/pull/3895

   Descriptions of the changes in this PR:
   
   ### Motivation
   
   read ahead only limits the number of entries, which is not precise enough. so we need to add the bytes limit to make the prefetch to be more controllable
   
   ### Changes
   
   1.  add dbStorage_readAheadCacheBatchBytesSize properties 
   2. when dbStorage_readAheadCacheBatchBytesSize is setted, use it to limit the bytes for read ahead entries.


-- 
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@bookkeeper.apache.org

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


[GitHub] [bookkeeper] StevenLuMT commented on pull request #3895: [feature] [server] add dbStorage_readAheadCacheBatchBytesSize properties when read ahead entries

Posted by "StevenLuMT (via GitHub)" <gi...@apache.org>.
StevenLuMT commented on PR #3895:
URL: https://github.com/apache/bookkeeper/pull/3895#issuecomment-1487866882

   > Do you have perf test results demonstrating improvements with this option?
   
   Thanks for reviewing , I will add unit test today @dlg99 


-- 
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@bookkeeper.apache.org

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


[GitHub] [bookkeeper] StevenLuMT commented on pull request #3895: [feature] [server] add dbStorage_readAheadCacheBatchBytesSize properties when read ahead entries

Posted by "StevenLuMT (via GitHub)" <gi...@apache.org>.
StevenLuMT commented on PR #3895:
URL: https://github.com/apache/bookkeeper/pull/3895#issuecomment-1486743335

   @eolivelli @dlg99 @zymap @Shoothzj @HQebupt @AnonHxy 
   needs review, thanks


-- 
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@bookkeeper.apache.org

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


[GitHub] [bookkeeper] StevenLuMT commented on a diff in pull request #3895: [feature] [server] add dbStorage_readAheadCacheBatchBytesSize properties when read ahead entries

Posted by "StevenLuMT (via GitHub)" <gi...@apache.org>.
StevenLuMT commented on code in PR #3895:
URL: https://github.com/apache/bookkeeper/pull/3895#discussion_r1153092216


##########
conf/bk_server.conf:
##########
@@ -749,6 +749,9 @@ gcEntryLogMetadataCacheEnabled=false
 # By default it will be allocated to 25% of the available direct memory
 # dbStorage_readAheadCacheMaxSizeMb=
 
+# How many entries' bytes to pre-fill in cache after a read cache miss
+# dbStorage_readAheadCacheBatchBytesSize=-1

Review Comment:
   > How about change the default value to 0? Because 0 will also disable this feature.
   > 
   > And we'd better add a comments like this: `Default is 0. 0 or less disables this feature`
   
   updated,thanks



-- 
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@bookkeeper.apache.org

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


[GitHub] [bookkeeper] StevenLuMT merged pull request #3895: [feature] [server] add dbStorage_readAheadCacheBatchBytesSize properties when read ahead entries

Posted by "StevenLuMT (via GitHub)" <gi...@apache.org>.
StevenLuMT merged PR #3895:
URL: https://github.com/apache/bookkeeper/pull/3895


-- 
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@bookkeeper.apache.org

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


[GitHub] [bookkeeper] StevenLuMT commented on a diff in pull request #3895: [feature] [server] add dbStorage_readAheadCacheBatchBytesSize properties when read ahead entries

Posted by "StevenLuMT (via GitHub)" <gi...@apache.org>.
StevenLuMT commented on code in PR #3895:
URL: https://github.com/apache/bookkeeper/pull/3895#discussion_r1151332312


##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java:
##########
@@ -695,6 +696,18 @@ private void fillReadAheadCache(long orginalLedgerId, long firstEntryId, long fi
         }
     }
 
+    private boolean chargeReadAheadCache(int currentReadAheadCount, long currentReadAheadBytes) {

Review Comment:
   > can we add unit tests for this ? you can make it a static method, it will be very easy to write an unit test.
   > the logic is not straightforward, a test with edge cases will help
   
   Thanks for reviewing , I will add unit test today @eolivelli 



-- 
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@bookkeeper.apache.org

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


[GitHub] [bookkeeper] StevenLuMT commented on a diff in pull request #3895: [feature] [server] add dbStorage_readAheadCacheBatchBytesSize properties when read ahead entries

Posted by "StevenLuMT (via GitHub)" <gi...@apache.org>.
StevenLuMT commented on code in PR #3895:
URL: https://github.com/apache/bookkeeper/pull/3895#discussion_r1161054617


##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java:
##########
@@ -695,6 +696,18 @@ private void fillReadAheadCache(long orginalLedgerId, long firstEntryId, long fi
         }
     }
 
+    private boolean chargeReadAheadCache(int currentReadAheadCount, long currentReadAheadBytes) {

Review Comment:
   > I saw the unit tests, very good.
   > 
   > But here I meant to add a "unit test" that tests this method (you can make it "static", no need to start a Bookie)
   
   updated,thanks



-- 
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@bookkeeper.apache.org

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


[GitHub] [bookkeeper] AnonHxy commented on a diff in pull request #3895: [feature] [server] add dbStorage_readAheadCacheBatchBytesSize properties when read ahead entries

Posted by "AnonHxy (via GitHub)" <gi...@apache.org>.
AnonHxy commented on code in PR #3895:
URL: https://github.com/apache/bookkeeper/pull/3895#discussion_r1152676436


##########
conf/bk_server.conf:
##########
@@ -749,6 +749,9 @@ gcEntryLogMetadataCacheEnabled=false
 # By default it will be allocated to 25% of the available direct memory
 # dbStorage_readAheadCacheMaxSizeMb=
 
+# How many entries' bytes to pre-fill in cache after a read cache miss
+# dbStorage_readAheadCacheBatchBytesSize=-1

Review Comment:
   How about change the default value to 0?  Because 0 will also disable this feature.
   
   And we'd better add a comments like this:
   `Default is 0. 0 or less disables this feature`



-- 
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@bookkeeper.apache.org

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


[GitHub] [bookkeeper] StevenLuMT commented on pull request #3895: [feature] [server] add dbStorage_readAheadCacheBatchBytesSize properties when read ahead entries

Posted by "StevenLuMT (via GitHub)" <gi...@apache.org>.
StevenLuMT commented on PR #3895:
URL: https://github.com/apache/bookkeeper/pull/3895#issuecomment-1500338340

   > perf test
   
   @dlg99 @hangc0276 add three testcase: 
   1. Regression test case: DbLedgerStorageReadCacheTest#chargeReadAheadCacheRegressionTest
   2. Unit test case: DbLedgerStorageReadCacheTest#chargeReadAheadCacheUnitTest
   3. Perf test case: DbLedgerStorageReadCacheTest#compareDiffReadAheadPerfTest


-- 
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@bookkeeper.apache.org

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


[GitHub] [bookkeeper] eolivelli commented on a diff in pull request #3895: [feature] [server] add dbStorage_readAheadCacheBatchBytesSize properties when read ahead entries

Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on code in PR #3895:
URL: https://github.com/apache/bookkeeper/pull/3895#discussion_r1153172267


##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java:
##########
@@ -695,6 +696,18 @@ private void fillReadAheadCache(long orginalLedgerId, long firstEntryId, long fi
         }
     }
 
+    private boolean chargeReadAheadCache(int currentReadAheadCount, long currentReadAheadBytes) {

Review Comment:
   I saw the unit tests, very good.
   
   But here I meant to add a "unit test" that tests this method (you can make it "static", no need to start a Bookie)



-- 
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@bookkeeper.apache.org

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


[GitHub] [bookkeeper] AnonHxy commented on a diff in pull request #3895: [feature] [server] add dbStorage_readAheadCacheBatchBytesSize properties when read ahead entries

Posted by "AnonHxy (via GitHub)" <gi...@apache.org>.
AnonHxy commented on code in PR #3895:
URL: https://github.com/apache/bookkeeper/pull/3895#discussion_r1153088803


##########
conf/bk_server.conf:
##########
@@ -749,6 +749,9 @@ gcEntryLogMetadataCacheEnabled=false
 # By default it will be allocated to 25% of the available direct memory
 # dbStorage_readAheadCacheMaxSizeMb=
 
+# How many entries' bytes to pre-fill in cache after a read cache miss,default is 0 or less disables this feature

Review Comment:
   ```suggestion
   # How many entries' bytes to pre-fill in cache after a read cache miss. Default is -1. 0 or less disables this feature
   ```



-- 
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@bookkeeper.apache.org

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


[GitHub] [bookkeeper] StevenLuMT commented on a diff in pull request #3895: [feature] [server] add dbStorage_readAheadCacheBatchBytesSize properties when read ahead entries

Posted by "StevenLuMT (via GitHub)" <gi...@apache.org>.
StevenLuMT commented on code in PR #3895:
URL: https://github.com/apache/bookkeeper/pull/3895#discussion_r1153091712


##########
conf/bk_server.conf:
##########
@@ -749,6 +749,9 @@ gcEntryLogMetadataCacheEnabled=false
 # By default it will be allocated to 25% of the available direct memory
 # dbStorage_readAheadCacheMaxSizeMb=
 
+# How many entries' bytes to pre-fill in cache after a read cache miss,default is 0 or less disables this feature

Review Comment:
   good catch



-- 
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@bookkeeper.apache.org

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


[GitHub] [bookkeeper] StevenLuMT commented on a diff in pull request #3895: [feature] [server] add dbStorage_readAheadCacheBatchBytesSize properties when read ahead entries

Posted by "StevenLuMT (via GitHub)" <gi...@apache.org>.
StevenLuMT commented on code in PR #3895:
URL: https://github.com/apache/bookkeeper/pull/3895#discussion_r1161804847


##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java:
##########
@@ -695,6 +696,19 @@ private void fillReadAheadCache(long orginalLedgerId, long firstEntryId, long fi
         }
     }
 
+    protected boolean chargeReadAheadCache(int currentReadAheadCount, long currentReadAheadBytes) {
+        // compatible with old logic
+        boolean chargeSizeCondition = currentReadAheadCount < readAheadCacheBatchSize
+                && currentReadAheadBytes < maxReadAheadBytesSize;
+        if (readAheadCacheBatchBytesSize > 0) {
+            // exact limits limit the size and count for each batch
+            chargeSizeCondition = currentReadAheadCount < readAheadCacheBatchSize

Review Comment:
   simplified logic,updated,thanks



-- 
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@bookkeeper.apache.org

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


[GitHub] [bookkeeper] zymap commented on a diff in pull request #3895: [feature] [server] add dbStorage_readAheadCacheBatchBytesSize properties when read ahead entries

Posted by "zymap (via GitHub)" <gi...@apache.org>.
zymap commented on code in PR #3895:
URL: https://github.com/apache/bookkeeper/pull/3895#discussion_r1151303957


##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java:
##########
@@ -695,6 +696,18 @@ private void fillReadAheadCache(long orginalLedgerId, long firstEntryId, long fi
         }
     }
 
+    private boolean chargeReadAheadCache(int currentReadAheadCount, long currentReadAheadBytes) {
+        // compatible with old logic
+        boolean chargeSizeCondition = currentReadAheadCount < readAheadCacheBatchSize
+                && currentReadAheadBytes < maxReadAheadBytesSize;
+        if (readAheadCacheBatchBytesSize > 0) {

Review Comment:
   We should also check it is smaller than the max read cache size.



-- 
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@bookkeeper.apache.org

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


[GitHub] [bookkeeper] eolivelli commented on a diff in pull request #3895: [feature] [server] add dbStorage_readAheadCacheBatchBytesSize properties when read ahead entries

Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on code in PR #3895:
URL: https://github.com/apache/bookkeeper/pull/3895#discussion_r1150557283


##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java:
##########
@@ -695,6 +696,18 @@ private void fillReadAheadCache(long orginalLedgerId, long firstEntryId, long fi
         }
     }
 
+    private boolean chargeReadAheadCache(int currentReadAheadCount, long currentReadAheadBytes) {

Review Comment:
   can we add unit tests for this ? you can make it a static method, it will be very easy to write an unit test.
   the logic is not straightforward, a test with edge cases will help



-- 
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@bookkeeper.apache.org

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


[GitHub] [bookkeeper] StevenLuMT commented on pull request #3895: [feature] [server] add dbStorage_readAheadCacheBatchBytesSize properties when read ahead entries

Posted by "StevenLuMT (via GitHub)" <gi...@apache.org>.
StevenLuMT commented on PR #3895:
URL: https://github.com/apache/bookkeeper/pull/3895#issuecomment-1490134921

   I have add a unit test(TestBookieImpl) to cover this function,and add the performance result to show this change is valuable.
   help me review,thanks @dlg99 @eolivelli @hangc0276 @zymap 


-- 
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@bookkeeper.apache.org

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


[GitHub] [bookkeeper] dlg99 commented on pull request #3895: [feature] [server] add dbStorage_readAheadCacheBatchBytesSize properties when read ahead entries

Posted by "dlg99 (via GitHub)" <gi...@apache.org>.
dlg99 commented on PR #3895:
URL: https://github.com/apache/bookkeeper/pull/3895#issuecomment-1487159978

   Do you have perf test results demonstrating improvements with this option?


-- 
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@bookkeeper.apache.org

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


[GitHub] [bookkeeper] StevenLuMT commented on a diff in pull request #3895: [feature] [server] add dbStorage_readAheadCacheBatchBytesSize properties when read ahead entries

Posted by "StevenLuMT (via GitHub)" <gi...@apache.org>.
StevenLuMT commented on code in PR #3895:
URL: https://github.com/apache/bookkeeper/pull/3895#discussion_r1151333443


##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java:
##########
@@ -695,6 +696,18 @@ private void fillReadAheadCache(long orginalLedgerId, long firstEntryId, long fi
         }
     }
 
+    private boolean chargeReadAheadCache(int currentReadAheadCount, long currentReadAheadBytes) {
+        // compatible with old logic
+        boolean chargeSizeCondition = currentReadAheadCount < readAheadCacheBatchSize
+                && currentReadAheadBytes < maxReadAheadBytesSize;
+        if (readAheadCacheBatchBytesSize > 0) {

Review Comment:
   Good catch, I will update it @zymap 



-- 
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@bookkeeper.apache.org

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


[GitHub] [bookkeeper] AnonHxy commented on a diff in pull request #3895: [feature] [server] add dbStorage_readAheadCacheBatchBytesSize properties when read ahead entries

Posted by "AnonHxy (via GitHub)" <gi...@apache.org>.
AnonHxy commented on code in PR #3895:
URL: https://github.com/apache/bookkeeper/pull/3895#discussion_r1152673795


##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java:
##########
@@ -695,6 +696,18 @@ private void fillReadAheadCache(long orginalLedgerId, long firstEntryId, long fi
         }
     }
 
+    private boolean chargeReadAheadCache(int currentReadAheadCount, long currentReadAheadBytes) {
+        // compatible with old logic
+        boolean chargeSizeCondition = currentReadAheadCount < readAheadCacheBatchSize
+                && currentReadAheadBytes < maxReadAheadBytesSize;
+        if (readAheadCacheBatchBytesSize > 0) {
+            // exact limits limit the size and count for each batch
+            chargeSizeCondition = currentReadAheadCount < readAheadCacheBatchSize
+                    && currentReadAheadBytes < readAheadCacheBatchBytesSize;
+        }

Review Comment:
   If the `readAheadCacheBatchBytesSize` must smaller than `maxReadAheadBytesSize`, maybe we could change    like this:
   ```suggestion
           if (chargeSizeCondition && readAheadCacheBatchBytesSize > 0) {
               // exact limits limit the size and count for each batch
               chargeSizeCondition = currentReadAheadBytes < readAheadCacheBatchBytesSize;
           }
   ```



-- 
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@bookkeeper.apache.org

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


[GitHub] [bookkeeper] StevenLuMT commented on a diff in pull request #3895: [feature] [server] add dbStorage_readAheadCacheBatchBytesSize properties when read ahead entries

Posted by "StevenLuMT (via GitHub)" <gi...@apache.org>.
StevenLuMT commented on code in PR #3895:
URL: https://github.com/apache/bookkeeper/pull/3895#discussion_r1153092987


##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java:
##########
@@ -695,6 +696,18 @@ private void fillReadAheadCache(long orginalLedgerId, long firstEntryId, long fi
         }
     }
 
+    private boolean chargeReadAheadCache(int currentReadAheadCount, long currentReadAheadBytes) {
+        // compatible with old logic
+        boolean chargeSizeCondition = currentReadAheadCount < readAheadCacheBatchSize
+                && currentReadAheadBytes < maxReadAheadBytesSize;
+        if (readAheadCacheBatchBytesSize > 0) {
+            // exact limits limit the size and count for each batch
+            chargeSizeCondition = currentReadAheadCount < readAheadCacheBatchSize
+                    && currentReadAheadBytes < readAheadCacheBatchBytesSize;
+        }

Review Comment:
   > If the `readAheadCacheBatchBytesSize` must smaller than `maxReadAheadBytesSize`, maybe we could change like this:
   
   The logic is not inherited, it cannot be changed like this, thanks



-- 
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@bookkeeper.apache.org

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


[GitHub] [bookkeeper] StevenLuMT commented on pull request #3895: [feature] [server] add dbStorage_readAheadCacheBatchBytesSize properties when read ahead entries

Posted by "StevenLuMT (via GitHub)" <gi...@apache.org>.
StevenLuMT commented on PR #3895:
URL: https://github.com/apache/bookkeeper/pull/3895#issuecomment-1500352697

   rerun failure checks


-- 
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@bookkeeper.apache.org

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


[GitHub] [bookkeeper] hangc0276 commented on a diff in pull request #3895: [feature] [server] add dbStorage_readAheadCacheBatchBytesSize properties when read ahead entries

Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on code in PR #3895:
URL: https://github.com/apache/bookkeeper/pull/3895#discussion_r1161791955


##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/storage/ldb/SingleDirectoryDbLedgerStorage.java:
##########
@@ -695,6 +696,19 @@ private void fillReadAheadCache(long orginalLedgerId, long firstEntryId, long fi
         }
     }
 
+    protected boolean chargeReadAheadCache(int currentReadAheadCount, long currentReadAheadBytes) {
+        // compatible with old logic
+        boolean chargeSizeCondition = currentReadAheadCount < readAheadCacheBatchSize
+                && currentReadAheadBytes < maxReadAheadBytesSize;
+        if (readAheadCacheBatchBytesSize > 0) {
+            // exact limits limit the size and count for each batch
+            chargeSizeCondition = currentReadAheadCount < readAheadCacheBatchSize

Review Comment:
   if (readAheadCacheBatchBytesSize > 0 && chargeSizeCondition) {
       chargeSizeCondition = chargeSizeCondition && currentReadAheadBytes < readCacheMaxSize;
   }



-- 
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@bookkeeper.apache.org

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