[GitHub] [bookkeeper] dlg99 commented on a change in pull request #2973: checkAllLedgers in Auditor supports read throttle

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

dlg99 commented on a change in pull request #2973:
URL: https://github.com/apache/bookkeeper/pull/2973#discussion_r779216068



##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerChecker.java
##########
@@ -51,6 +51,14 @@
     public final BookieClient bookieClient;
     public final BookieWatcher bookieWatcher;
 
+    private static int averageEntrySize;
+
+    private static final int INITIAL_AVERAGE_ENTRY_SIZE = 1024;

Review comment:
       consider simplifying by using number of entries in flight (using Semaphore, releasing when processed) instead of guessing avg sizes and rate limiting. Or rate limit by number of entries.
   This also removes need in synchronization.
   
   One ledger may have entries of 512K, next one of the 1K, third one is mixed. 
   I don't see how tracking avg size significantly helps in this case, especially if the backpressure is not enabled.  
   

##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerChecker.java
##########
@@ -76,6 +84,11 @@
         @Override
         public void readEntryComplete(int rc, long ledgerId, long entryId,
                 ByteBuf buffer, Object ctx) {
+            if (readThrottle != null && buffer != null) {
+                int readSize = buffer.readableBytes();
+                averageEntrySize = (int) (averageEntrySize * AVERAGE_ENTRY_SIZE_RATIO

Review comment:
       async code, concurrent reads, plus updates of non-volatile field/later reads from non-volatile field. 
   You'll need to add synchronization around use of this field.




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

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