ctubbsii commented on code in PR #5999:
URL: https://github.com/apache/accumulo/pull/5999#discussion_r2695296196
##########
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java:
##########
@@ -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
Review Comment:
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.
##########
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloConfigImpl.java:
##########
@@ -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);
+ }
Review Comment:
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.
##########
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloConfigImpl.java:
##########
@@ -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);
Review Comment:
This changes behavior. "set" as the verb means "use these and only these",
but this changes behavior to "put", which appends, rather than replaces.
##########
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloConfigImpl.java:
##########
@@ -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");
Review Comment:
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...`
--
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]
