[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3725: Use memory percentage for GC batch size

["ctubbsii (via GitHub)" <gi...@apache.org>]

ctubbsii commented on code in PR #3725:
URL: https://github.com/apache/accumulo/pull/3725#discussion_r1309026653


##########
server/gc/src/main/java/org/apache/accumulo/gc/GCRun.java:
##########
@@ -101,6 +101,9 @@ public GCRun(Ample.DataLevel level, ServerContext context) {
     this.level = level;
     this.context = context;
     this.config = context.getConfiguration();
+    log.info("GC candidate batch size = {} bytes ({} of GC memory)",
+        config.getAsBytes(Property.GC_CANDIDATE_BATCH_SIZE),
+        config.get(Property.GC_CANDIDATE_BATCH_SIZE));

Review Comment:
   Probably shouldn't log here, as it will log 3x every cycle.



##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -867,8 +867,8 @@ public enum Property {
   GC_PREFIX("gc.", null, PropertyType.PREFIX,
       "Properties in this category affect the behavior of the accumulo garbage collector.",
       "1.3.5"),
-  GC_CANDIDATE_BATCH_SIZE("gc.candidate.batch.size", "8M", PropertyType.BYTES,
-      "The batch size used for garbage collection.", "2.1.0"),
+  GC_CANDIDATE_BATCH_SIZE("gc.candidate.batch.size", "50%", PropertyType.MEMORY,
+      "The amount of memory used to calculate batch size for garbage collection.", "2.1.0"),

Review Comment:
   ```suggestion
         "The amount of memory used as the batch size for garbage collection.", "2.1.0"),
   ```



##########
server/gc/src/main/java/org/apache/accumulo/gc/SimpleGarbageCollector.java:
##########
@@ -90,7 +90,8 @@ public class SimpleGarbageCollector extends AbstractServer implements Iface {
     log.info("start delay: {} milliseconds", getStartDelay());
     log.info("time delay: {} milliseconds", gcDelay);
     log.info("safemode: {}", inSafeMode());
-    log.info("candidate batch size: {} bytes", getCandidateBatchSize());
+    log.info("GC candidate batch size: {} bytes ({} of GC memory)", getCandidateBatchSize(),

Review Comment:
   GC is redundant.
   
   ```suggestion
       log.info("candidate batch size: {} bytes ({} of memory)", getCandidateBatchSize(),
   ```
   
   I'm not sure the parenthetical addition is adding much either. The site configuration is already logged on startup, so they can already see the String value in the logs.



##########
test/src/main/java/org/apache/accumulo/test/functional/GarbageCollectorIT.java:
##########
@@ -310,6 +311,33 @@ public void testProperPortAdvertisement() throws Exception {
     }
   }
 
+  // Verify that the GC_CANDIDATE_BATCH_SIZE can use percentage rather than a specific byte value.
+  @Test
+  public void gcTestSetGCBatchSizeAsMemoryValue() throws Exception {
+    killMacGc();
+    Thread.sleep(SECONDS.toMillis(15));

Review Comment:
   Instead of doing arbitrary sleep durations, should use Wait.waitFor, which can wait up to a max duration (scaled by timeout.factor) for the condition being tested to happen.
   
   But actually, I don't think this test is needed at all. It's just testing the ConfigurationTypeHelper.getMemoryAsBytes, which already has test coverage.
   
   I think it can just be deleted.



-- 
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: notifications-unsubscribe@accumulo.apache.org

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