[GitHub] [bookkeeper] nicoloboschi commented on a change in pull request #2802: Add auditor get ledger throttle to avoid auto recovery zk session expire

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

nicoloboschi commented on a change in pull request #2802:
URL: https://github.com/apache/bookkeeper/pull/2802#discussion_r713961843



##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/Auditor.java
##########
@@ -347,6 +348,7 @@ public Auditor(final String bookieIdentifier,
         this.numLedgersFoundHavingLessThanAQReplicasOfAnEntry = new AtomicInteger(0);
         this.numLedgersHavingLessThanWQReplicasOfAnEntryGuageValue = new AtomicInteger(0);
         this.numLedgersFoundHavingLessThanWQReplicasOfAnEntry = new AtomicInteger(0);
+        this.semaphore = new Semaphore(conf.getAuditorGetLedgerSemaphore());

Review comment:
       maybe it is better to validate this value (must be greater than zero) and throw an exception with a useful error message (and indicate the invalid config prop name)

##########
File path: bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/AuditorPeriodicCheckTest.java
##########
@@ -365,6 +366,36 @@ public void run() {
         }
     }
 
+    @Test
+    public void testGetLedgerFromZookeeperThrottled() throws Exception {
+        for (AuditorElector e : auditorElectors.values()) {
+            e.shutdown();
+        }
+
+        final int numberLedgers = 100;
+        // write ledgers into bookkeeper cluster
+        for (int i = 0; i < numberLedgers; ++i) {
+            LedgerHandle lh = bkc.createLedger(3, 3, DigestType.CRC32, "passwd".getBytes());
+            for (int j = 0; j < 5; j++) {
+                lh.addEntry("testdata".getBytes());
+            }
+            lh.close();
+        }
+
+        // create auditor and call `checkAllLedgers`
+        ServerConfiguration configuration = confByIndex(0);
+        configuration.setAuditorGetLedgerSemaphore(5);
+        try (final Auditor auditor = new Auditor(
+            BookieImpl.getBookieId(configuration).toString(),
+            configuration, NullStatsLogger.INSTANCE)) {
+            auditor.checkAllLedgers();

Review comment:
       +1 for this




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