keith-turner commented on code in PR #4080:
URL: https://github.com/apache/accumulo/pull/4080#discussion_r1430704991
##########
test/src/main/java/org/apache/accumulo/test/compaction/CompactionExecutorIT.java:
##########
@@ -67,36 +69,49 @@
import org.apache.accumulo.core.spi.compaction.CompactionKind;
import org.apache.accumulo.core.spi.compaction.CompactionPlan;
import org.apache.accumulo.core.spi.compaction.CompactionPlanner;
+import org.apache.accumulo.harness.MiniClusterConfigurationCallback;
import org.apache.accumulo.harness.SharedMiniClusterBase;
+import org.apache.accumulo.minicluster.ServerType;
+import org.apache.accumulo.miniclusterImpl.MiniAccumuloConfigImpl;
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.RawLocalFileSystem;
import org.apache.hadoop.io.Text;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeAll;
-import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
-@Disabled // ELASTICITY_TODO
+import com.google.gson.JsonArray;
+import com.google.gson.JsonElement;
+
public class CompactionExecutorIT extends SharedMiniClusterBase {
+ public static final List<String> compactionGroups = new LinkedList<>();
public static class TestPlanner implements CompactionPlanner {
+ private static class ExecutorConfig {
+ String name;
+ String type;
Review Comment:
Does not seem like these two fields are read, but not sure. If they are not
read could they be dropped?
##########
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterControl.java:
##########
@@ -254,6 +254,25 @@ public void stop(ServerType server) throws IOException {
stop(server, null);
}
+ public void stopCompactorGroup(String compactorResourceGroup) throws
IOException {
+ synchronized (compactorProcesses) {
+ var group = compactorProcesses.get(compactorResourceGroup);
+ if (group == null) {
+ return;
+ }
+ group.forEach(process -> {
+ try {
+ cluster.stopProcessWithTimeout(process, 30, TimeUnit.SECONDS);
+ } catch (ExecutionException | TimeoutException e) {
+ log.warn("Compactor did not fully stop after 30 seconds", e);
Review Comment:
This is following the pattern of other code, so I think this is fine here.
This general pattern seems like it could cause trouble. If a test wants to
stop something and runs into this and fails later because it was not stopped
could make the test harder to debug.
##########
test/src/main/java/org/apache/accumulo/test/compaction/CompactionExecutorIT.java:
##########
@@ -180,6 +212,18 @@ public static void teardown() {
@AfterEach
public void cleanup() {
+ var iter = compactionGroups.iterator();
+ while (iter.hasNext()) {
+ var group = iter.next();
+
getCluster().getConfig().getClusterServerConfiguration().addCompactorResourceGroup(group,
0);
Review Comment:
If its quick to explain, I am curious about why this is called with zero. If
its not quick to explain let me know and I can look at the code.
##########
test/src/main/java/org/apache/accumulo/test/compaction/CompactionExecutorIT.java:
##########
@@ -180,6 +212,18 @@ public static void teardown() {
@AfterEach
public void cleanup() {
+ var iter = compactionGroups.iterator();
+ while (iter.hasNext()) {
+ var group = iter.next();
+
getCluster().getConfig().getClusterServerConfiguration().addCompactorResourceGroup(group,
0);
+ try {
+ getCluster().getClusterControl().stopCompactorGroup(group);
+ } catch (Exception e) {
+ // Exception is encountered but ignored.
Review Comment:
Why ignore this? Was wondering if it would be useful to log it.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
