Make Mini Accumulo hardcoded JVM options mutable #5999
Conversation
Processes spawned by MiniAccumuloCluster have a pre-configured, but not mutable, set of JVM options. Specifically they are '-XX:+PerfDisableSharedMem' and '-XX:+AlwaysPreTouch'. This change makes the set of JVM options configurable.
kevinrr888
left a comment
kevinrr888
left a comment
There was a problem hiding this comment.
looks good to me, not sure if we want this to target 2.1.5 but I don't have any problems with it
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloConfigImpl.java
Show resolved
Hide resolved
ddanielr
left a comment
ddanielr
left a comment
There was a problem hiding this comment.
Tested and confirmed that the get, set, and remove methods for JvmOptions all work.
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloConfigImpl.java
Show resolved
Hide resolved
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java
Outdated
Show resolved
Hide resolved
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloConfigImpl.java
Show resolved
Hide resolved
ctubbsii
left a comment
ctubbsii
left a comment
There was a problem hiding this comment.
This PR has already been merged, but I noticed a bug with the -D being added twice, and the mismatch on the setProperties API name with its behavior, so I felt this needed an after-the-fact review.
| */ | ||
| public void setSystemProperties(Map<String,String> systemProperties) { | ||
| this.systemProperties = new HashMap<>(systemProperties); | ||
| this.systemProperties.putAll(systemProperties); |
There was a problem hiding this 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.
| this.systemProperties.put("-Dapple.awt.UIElement", "true"); | ||
| this.systemProperties.put("-Djava.net.preferIPv4Stack", "true"); |
There was a problem hiding this comment.
These were moved from the hard-coded set into the system properties set that the user inserts. This has two problems:
- 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
- Previously, the
-Dwas needed because they were being added separately from the system properties, but now, as part of the system properties, the-Dis redundant, because the system properties set prepends the-Dwhen constructing the command-line. So, these will get malformed as-D-D...
| // @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 |
There was a problem hiding this 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.
| /** | ||
| * Get the set of JVM options | ||
| * | ||
| * @return set of options | ||
| * @since 2.1.5 | ||
| */ | ||
| public Set<String> getJvmOptions() { | ||
| return new HashSet<>(jvmOptions); | ||
| } |
There was a problem hiding this 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.
5 participants
Processes spawned by MiniAccumuloCluster have a pre-configured, but not mutable, set of JVM options. Specifically they are '-XX:+PerfDisableSharedMem' and '-XX:+AlwaysPreTouch'. This change makes the set of JVM options configurable.