[bookkeeper] branch master updated: Defect-Fix Compaction status report into correct buckets. (#2645)

[zh...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

zhaijia pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/bookkeeper.git


The following commit(s) were added to refs/heads/master by this push:
     new 2722c9b  Defect-Fix Compaction status report into correct buckets. (#2645)
2722c9b is described below

commit 2722c9bfed3396f7fa0785c5e8e412fd412f03c0
Author: Kevin Wilson <kl...@comcast.net>
AuthorDate: Thu Apr 1 00:42:44 2021 -0600

    Defect-Fix Compaction status report into correct buckets. (#2645)
    
    The compaction status report is off by 1.
    
    When compaction is enabled, the 10% log is always empty. Setting the compaction threshold to .2 we were seeing the number of compacted entrylogs that were removed, match the sum of the 20% and 30% buckets. While the 10% bucket remained empty. Increasing the threshold to .3 we would see the sum of the first 4 buckets compacted (10%-40%). Also with the 10% bucket empty.
    
    Upon inspection of the code, the code is using the .ceil function to calculate the bucket index to be updated. This means that 10% bucket will never be used. However, there could be a rare 100% case in which the code runs into a index out of bounds exception when the entrylog is actually 100% used. The index calculation should be using floor and not the ceil call using the next index lower. So usage of .00 to .9 ends up in the 0 index instead of the current calculation which puts the  [...]
    
    This will leave the 100% index empty most of the time, as rarely will the entry logs be 100% used.
---
 .../bookkeeper/bookie/GarbageCollectorThread.java  | 17 ++++++++++--
 .../bookie/GarbageCollectorThreadTest.java         | 32 +++++++++++++++++++++-
 2 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java
index c242778..e2a75ac 100644
--- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java
+++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java
@@ -454,9 +454,7 @@ public class GarbageCollectorThread extends SafeRunnable {
         int[] entryLogUsageBuckets = new int[numBuckets];
 
         for (EntryLogMetadata meta : logsToCompact) {
-            int bucketIndex = Math.min(
-                    numBuckets - 1,
-                    (int) Math.ceil(meta.getUsage() * numBuckets));
+            int bucketIndex = calculateUsageIndex(numBuckets, meta.getUsage());
             entryLogUsageBuckets[bucketIndex]++;
 
             if (meta.getUsage() >= threshold) {
@@ -480,6 +478,19 @@ public class GarbageCollectorThread extends SafeRunnable {
     }
 
     /**
+     * Calculate the index for the batch based on the usage between 0 and 1.
+     *
+     * @param numBuckets Number of reporting buckets.
+     * @param usage 0.0 - 1.0 value representing the usage of the entry log.
+     * @return index based on the number of buckets The last bucket will have the 1.0 if added.
+     */
+    int calculateUsageIndex(int numBuckets, double usage) {
+        return Math.min(
+                numBuckets - 1,
+                (int) Math.floor(usage * numBuckets));
+    }
+
+    /**
      * Shutdown the garbage collector thread.
      *
      * @throws InterruptedException if there is an exception stopping gc thread.
diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/GarbageCollectorThreadTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/GarbageCollectorThreadTest.java
index be6c81b..d15db12 100644
--- a/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/GarbageCollectorThreadTest.java
+++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/GarbageCollectorThreadTest.java
@@ -32,18 +32,21 @@ import java.util.concurrent.atomic.AtomicBoolean;
 import org.apache.bookkeeper.conf.ServerConfiguration;
 import org.apache.bookkeeper.meta.LedgerManager;
 import org.apache.bookkeeper.stats.StatsLogger;
+import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 import org.mockito.InjectMocks;
 import org.mockito.Mock;
 import org.mockito.Spy;
 import org.powermock.reflect.Whitebox;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * Unit test for {@link GarbageCollectorThread}.
  */
 public class GarbageCollectorThreadTest {
-
+    private static final Logger LOG = LoggerFactory.getLogger(GarbageCollectorThreadTest.class);
     @InjectMocks
     @Spy
     private GarbageCollectorThread mockGCThread;
@@ -78,4 +81,31 @@ public class GarbageCollectorThreadTest {
         mockGCThread.compactEntryLog(new EntryLogMetadata(9999));
         assertFalse(compacting.get());
     }
+
+    @Test
+    public void testCalculateUsageBucket() {
+        // Valid range for usage is [0.0 to 1.0]
+        final int numBuckets = 10;
+        int[] usageBuckets = new int[numBuckets];
+        for (int i = 0; i < numBuckets; i++) {
+            usageBuckets[i] = 0;
+        }
+
+        int items = 10000;
+        for (int item = 0; item <= items; item++) {
+            double usage = ((double) item / (double) items);
+            int index = mockGCThread.calculateUsageIndex(numBuckets, usage);
+            Assert.assertFalse("Boundary condition exceeded", index < 0 || index >= numBuckets);
+            LOG.debug("Mapped {} usage to {}}\n", usage, index);
+            usageBuckets[index]++;
+        }
+        LOG.info(
+                "Compaction: entry log usage buckets[10% 20% 30% 40% 50% 60% 70% 80% 90% 100%] = {}",
+                usageBuckets);
+        int sum = 0;
+        for (int i = 0; i < numBuckets; i++) {
+            sum += usageBuckets[i];
+        }
+        Assert.assertEquals("Incorrect number of items", items + 1, sum);
+    }
 }