Skip to content

Make Mini Accumulo hardcoded JVM options mutable #5999

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dlmarion merged 2 commits into from
Dec 3, 2025
+47 −13
Merged

Make Mini Accumulo hardcoded JVM options mutable #5999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
61f8990
Make Mini Accumulo hardcoded JVM options mutable
dlmarion Dec 3, 2025
348d747
Implemented PR suggestions
dlmarion Dec 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 4 additions & 11 deletions minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,21 +348,14 @@ private ProcessInfo _exec(Class<?> clazz, List<String> extraJvmOpts, String... a
String javaBin = javaHome + File.separator + "bin" + File.separator + "java";

var basicArgs = Stream.of(javaBin, "-Dproc=" + clazz.getSimpleName());
var jvmArgs = extraJvmOpts.stream();
var propsArgs = config.getSystemProperties().entrySet().stream()
var jvmOptions = Stream.concat(config.getJvmOptions().stream(), extraJvmOpts.stream());
var systemProps = config.getSystemProperties().entrySet().stream()
.map(e -> String.format("-D%s=%s", e.getKey(), e.getValue()));

// @formatter:off
var hardcodedArgs = Stream.of(
"-Dapple.awt.UIElement=true",
"-Djava.net.preferIPv4Stack=true",
"-XX:+PerfDisableSharedMem",
"-XX:+AlwaysPreTouch",
Main.class.getName(), clazz.getName());
// @formatter:on
Comment on lines -355 to -362
Copy link
Member

@ctubbsii ctubbsii Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hard-coded set was intentionally set after all user-specified options, because they were intended to be not override-able.

I think for some of these (perhaps all of them), it's okay to be able to override, but they should be able to be overridden by just adding their opposite, as in -XX:-PerfDisableSharedMem (so long as the override occurs later in the command line).

I think the -XX args should be override-able, but the -D ones here were intended to be not overrideable, because we didn't want the user to have to specify them each time they used setProperties(), because setProperties() was intended to replace, not append, to properties.

var classArgs = Stream.of(Main.class.getName(), clazz.getName());

// concatenate all the args sources into a single list of args
var argList = Stream.of(basicArgs, jvmArgs, propsArgs, hardcodedArgs, Stream.of(args))
var argList = Stream.of(basicArgs, jvmOptions, systemProps, classArgs, Stream.of(args))
.flatMap(Function.identity()).collect(toList());
ProcessBuilder builder = new ProcessBuilder(argList);

Expand Down
45 changes: 43 additions & 2 deletions minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloConfigImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,12 @@
import java.io.IOException;
import java.util.EnumMap;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Objects;
import java.util.Set;
import java.util.function.Consumer;

import org.apache.accumulo.compactor.Compactor;
Expand Down Expand Up @@ -86,7 +88,8 @@ public class MiniAccumuloConfigImpl {
MONITOR, Monitor.class, ZOOKEEPER, ZooKeeperServerMain.class, TABLET_SERVER,
TabletServer.class, SCAN_SERVER, ScanServer.class, COMPACTOR, Compactor.class));
private boolean jdwpEnabled = false;
private Map<String,String> systemProperties = new HashMap<>();
private final Map<String,String> systemProperties = new HashMap<>();
private final Set<String> jvmOptions = new HashSet<>();

private String instanceName = "miniInstance";
private String rootUserName = "root";
Expand Down Expand Up @@ -131,6 +134,11 @@ public class MiniAccumuloConfigImpl {
public MiniAccumuloConfigImpl(File dir, String rootPassword) {
this.dir = dir;
this.rootPassword = rootPassword;
// Set default options
this.jvmOptions.add("-XX:+PerfDisableSharedMem");
this.jvmOptions.add("-XX:+AlwaysPreTouch");
this.systemProperties.put("-Dapple.awt.UIElement", "true");
this.systemProperties.put("-Djava.net.preferIPv4Stack", "true");
Comment on lines +140 to +141
Copy link
Member

@ctubbsii ctubbsii Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These were moved from the hard-coded set into the system properties set that the user inserts. This has two problems:

  1. Previously these were forced to appear after the user-specified properties, because we didn't want the user to override them, but now they can, and
  2. Previously, the -D was needed because they were being added separately from the system properties, but now, as part of the system properties, the -D is redundant, because the system properties set prepends the -D when constructing the command-line. So, these will get malformed as -D-D...

}

/**
Expand Down Expand Up @@ -669,7 +677,7 @@ public File getClientPropsFile() {
* @since 1.6.0
*/
public void setSystemProperties(Map<String,String> systemProperties) {
this.systemProperties = new HashMap<>(systemProperties);
this.systemProperties.putAll(systemProperties);
Copy link
Member

@ctubbsii ctubbsii Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes behavior. "set" as the verb means "use these and only these", but this changes behavior to "put", which appends, rather than replaces.

}

/**
Expand All @@ -681,6 +689,39 @@ public Map<String,String> getSystemProperties() {
return new HashMap<>(systemProperties);
}

/**
* Add a JVM option to the spawned JVM processes. The default set of JVM options contains
* '-XX:+PerfDisableSharedMem' and '-XX:+AlwaysPreTouch'
*
* @param option JVM option
* @since 2.1.5
*/
public void addJvmOption(String option) {
this.jvmOptions.add(option);
}

/**
* Remove an option from the set of JVM options. Only options that match the {@code option}
* exactly will be removed.
*
* @param option JVM option
* @return true if removed, false if not removed
* @since 2.1.5
*/
public boolean removeJvmOption(String option) {
return this.jvmOptions.remove(option);
}

/**
* Get the set of JVM options
*
* @return set of options
* @since 2.1.5
*/
public Set<String> getJvmOptions() {
return new HashSet<>(jvmOptions);
}
Comment on lines +715 to +723
Copy link
Member

@ctubbsii ctubbsii Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably don't need 3 separate methods for this Impl class. We can just expose the underlying jvmOptions, and mutate them, rather than having separate getter, setter, and remover methods. Or, we could reduce it to two by using a mutateJvmOptions method or similar. In any case, this seems like a heavy-weight change to an internal API for something that is almost never used.


/**
* Gets the classpath elements to use when spawning processes.
*
Expand Down