ctubbsii commented on code in PR #6062:
URL: https://github.com/apache/accumulo/pull/6062#discussion_r2695983497
##########
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java:
##########
@@ -347,10 +347,18 @@ private ProcessInfo _exec(Class<?> clazz, List<String>
extraJvmOpts, String... a
String javaHome = System.getProperty("java.home");
String javaBin = javaHome + File.separator + "bin" + File.separator +
"java";
+ Stream<String> defaultJvmOpts = Stream.of("-XX:+PerfDisableSharedMem",
"-XX:+AlwaysPreTouch");
+ Map<String,String> defaultSystemProps =
+ Map.of("-Dapple.awt.UIElement", "true", "-Djava.net.preferIPv4Stack",
"true");
+
var basicArgs = Stream.of(javaBin, "-Dproc=" + clazz.getSimpleName());
- 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()));
+
+ var jvmOptions = Stream.concat(Stream.concat(defaultJvmOpts,
config.getJvmOptions().stream()),
+ extraJvmOpts.stream());
+ var systemProps = Stream
+ .concat(defaultSystemProps.entrySet().stream(),
+ config.getSystemProperties().entrySet().stream())
+ .map(e -> String.format("%s=%s", e.getKey(), e.getValue()));
Review Comment:
This drops the `-D`, but it means that users need to add it to their keys
when they put it in their map, as in:
`cfg.setSystemProperties(Map.of("-DsomeKey", "someValue));`
That doesn't quite make sense. They should be prepended to the key=value
when the command-line is being constructed, not expected to be added to the key
by the user. The redundant one can be dropped from the defaults instead.
##########
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java:
##########
@@ -347,10 +347,18 @@ private ProcessInfo _exec(Class<?> clazz, List<String>
extraJvmOpts, String... a
String javaHome = System.getProperty("java.home");
String javaBin = javaHome + File.separator + "bin" + File.separator +
"java";
+ Stream<String> defaultJvmOpts = Stream.of("-XX:+PerfDisableSharedMem",
"-XX:+AlwaysPreTouch");
Review Comment:
This is fine since the stream isn't reused, but it might be better to put
these in a `Set.of` outside the method, and call `.stream()` inside the method,
to reuse the set.
However, I don't even know why these are here as default options. There's no
documentation explaining why they were added. They were added in
https://issues.apache.org/jira/browse/ACCUMULO-3699, but no specific
explanation about what they were expected to achieve. They may be OBE.
##########
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloConfigImpl.java:
##########
@@ -677,7 +672,7 @@ public File getClientPropsFile() {
* @since 1.6.0
*/
public void setSystemProperties(Map<String,String> systemProperties) {
- this.systemProperties.putAll(systemProperties);
+ this.systemProperties = new HashMap<>(systemProperties);
Review Comment:
👍
--
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]
