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-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
