ddanielr commented on code in PR #4080:
URL: https://github.com/apache/accumulo/pull/4080#discussion_r1430783736
##########
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.
I agree that it wouldn't be immediately obvious that the shutdown failing
caused an error.
To help with that, we could either throw an exception on failed process stop
or this method could return an exit code.
I'm leaning towards the exit code route since it cuts down on the amount of
try/catch statements in a given test and also allows the test to ignore the
output of this method if they wanted.
Do you have an opinion?
--
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]
