1996fanrui commented on code in PR #24098:
URL: https://github.com/apache/flink/pull/24098#discussion_r1453054820
##########
flink-core/src/main/java/org/apache/flink/configuration/CoreOptions.java:
##########
@@ -282,6 +282,42 @@ public static String[] mergeListsToArray(List<String>
base, List<String> append)
"Java options to start the JVM of
the Flink SQL Gateway with.")
.build());
+ public static final ConfigOption<String> FLINK_DEFAULT_JVM_OPTIONS =
+ ConfigOptions.key("env.java.default-opts.all")
+ .stringType()
+ .defaultValue("")
+ .withDescription(
+ Description.builder()
+ .text(
+ "A string of default JVM options
to prepend to \""
+ + FLINK_JVM_OPTIONS.key()
+ + "\". This is intended to
be set by administrators.")
Review Comment:
```suggestion
"A string of default JVM options
to prepend to %s. This is intended to be set by administrators."
, code(FLINK_JVM_OPTIONS.key()))
```
How about it?
##########
flink-dist/src/main/flink-bin/bin/config.sh:
##########
@@ -186,6 +186,9 @@ KEY_ENV_JAVA_OPTS_TM="env.java.opts.taskmanager"
KEY_ENV_JAVA_OPTS_HS="env.java.opts.historyserver"
KEY_ENV_JAVA_OPTS_CLI="env.java.opts.client"
KEY_ENV_JAVA_OPTS_SQL_GATEWAY="env.java.opts.sql-gateway"
+KEY_ENV_JAVA_DEFAULT_OPTS="env.java.default-opts.all"
Review Comment:
`"env.java.opts.all"` are supported in `pyflink_gateway_server.py`,
`"env.java.default-opts.all"` should be considered there as well.
##########
flink-yarn/src/main/java/org/apache/flink/yarn/YarnClusterDescriptor.java:
##########
@@ -1834,9 +1834,23 @@ ContainerLaunchContext setupApplicationMasterContainer(
// ------------------ Prepare Application Master Container
------------------------------
// respect custom JVM options in the YAML file
- String javaOpts =
flinkConfiguration.getString(CoreOptions.FLINK_JVM_OPTIONS);
- if
(flinkConfiguration.getString(CoreOptions.FLINK_JM_JVM_OPTIONS).length() > 0) {
- javaOpts += " " +
flinkConfiguration.getString(CoreOptions.FLINK_JM_JVM_OPTIONS);
+ String javaOpts = "";
+ String defaultJvmOpts =
flinkConfiguration.getString(CoreOptions.FLINK_DEFAULT_JVM_OPTIONS);
+ String jvmOpts =
flinkConfiguration.getString(CoreOptions.FLINK_JVM_OPTIONS);
+ String defaultJmJvmOpts =
+
flinkConfiguration.getString(CoreOptions.FLINK_DEFAULT_JM_JVM_OPTIONS);
+ String jmJvmOpts =
flinkConfiguration.getString(CoreOptions.FLINK_JM_JVM_OPTIONS);
+ if (!defaultJvmOpts.isEmpty()) {
+ javaOpts += " " + defaultJvmOpts;
+ }
+ if (!jvmOpts.isEmpty()) {
+ javaOpts += " " + jvmOpts;
+ }
+ if (!defaultJmJvmOpts.isEmpty()) {
+ javaOpts += " " + defaultJmJvmOpts;
+ }
+ if (!jmJvmOpts.isEmpty()) {
+ javaOpts += " " + jmJvmOpts;
}
Review Comment:
nit:
Using the `StringBuilder` to build the javaOpts, and we can define a method
to avoid repetitive code here.
```
private void method(StringBuilder sb, Configuration conf,
ConfigOption<String> option) {
final String javaOpts = conf.get(option);
if(javaOpts == null || javaOpts.isEmpty()) {
return;
}
sb.append(' ');
sb.append(javaOpts);
}
```
##########
flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/BootstrapTools.java:
##########
@@ -191,9 +191,22 @@ public static String getTaskManagerShellCommand(
startCommandValues.put(
"jvmmem",
ProcessMemoryUtils.generateJvmParametersStr(taskExecutorProcessSpec));
- String javaOpts = flinkConfig.getString(CoreOptions.FLINK_JVM_OPTIONS);
- if (flinkConfig.getString(CoreOptions.FLINK_TM_JVM_OPTIONS).length() >
0) {
- javaOpts += " " +
flinkConfig.getString(CoreOptions.FLINK_TM_JVM_OPTIONS);
+ String javaOpts = "";
+ String defaultJvmOpts =
flinkConfig.getString(CoreOptions.FLINK_DEFAULT_JVM_OPTIONS);
+ String jvmOpts = flinkConfig.getString(CoreOptions.FLINK_JVM_OPTIONS);
+ String defaultTmJvmOpts =
flinkConfig.getString(CoreOptions.FLINK_DEFAULT_TM_JVM_OPTIONS);
+ String tmJvmOpts =
flinkConfig.getString(CoreOptions.FLINK_TM_JVM_OPTIONS);
+ if (!defaultJvmOpts.isEmpty()) {
+ javaOpts += " " + defaultJvmOpts;
+ }
+ if (!jvmOpts.isEmpty()) {
+ javaOpts += " " + jvmOpts;
+ }
+ if (!defaultTmJvmOpts.isEmpty()) {
+ javaOpts += " " + defaultTmJvmOpts;
+ }
+ if (!tmJvmOpts.isEmpty()) {
+ javaOpts += " " + tmJvmOpts;
}
Review Comment:
This part is total same with Yarn side, could we extract a common method?
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
