szilard-nemeth commented on code in PR #6118:
URL: https://github.com/apache/hadoop/pull/6118#discussion_r1339225210
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/resources/yarn-default.xml:
##########
@@ -1478,6 +1478,17 @@
<value>-Xmx256m</value>
</property>
+ <property>
+
<name>yarn.nodemanager.container-localizer.java.opts.add-exports-as-default</name>
+ <value>true</value>
+ <description>Since on JDK17 it's no longer possible to use the reflection
API to
+ access public fields and methods add-exports flags should be added to
container
Review Comment:
Nit: comma should be added after: "field and methods"
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/resources/yarn-default.xml:
##########
@@ -1478,6 +1478,17 @@
<value>-Xmx256m</value>
</property>
+ <property>
+
<name>yarn.nodemanager.container-localizer.java.opts.add-exports-as-default</name>
+ <value>true</value>
+ <description>Since on JDK17 it's no longer possible to use the reflection
API to
Review Comment:
Nit: It's --> It is (more formal)
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/conf/YarnConfiguration.java:
##########
@@ -2228,6 +2228,14 @@ public static boolean isAclEnabled(Configuration conf) {
public static final String NM_CONTAINER_LOCALIZER_JAVA_OPTS_DEFAULT =
"-Xmx256m";
+ /*
+ * Flag to indicate whether JDK17's required add-exports flags should be
added to
+ * container localizers regardless of the user specified java opts.
Review Comment:
Nit: JAVA_OPTS?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/ContainerLocalizer.java:
##########
@@ -102,6 +102,9 @@ public class ContainerLocalizer {
private static final FsPermission USERCACHE_FOLDER_PERMS =
new FsPermission((short) 0755);
public static final String CSI_VOLIUME_MOUNTS_ROOT = "csivolumes";
+ private static final String ADDITIONAL_JDK17_PLUS_OPTIONS =
Review Comment:
I can't see "--add-opens=java.base/java.lang=ALL-UNNAMED " here whereas I
can see it in ContainerLaunch.java?
Can you explain with a code comment why is this difference?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/resources/yarn-default.xml:
##########
@@ -1478,6 +1478,17 @@
<value>-Xmx256m</value>
</property>
+ <property>
+
<name>yarn.nodemanager.container-localizer.java.opts.add-exports-as-default</name>
+ <value>true</value>
+ <description>Since on JDK17 it's no longer possible to use the reflection
API to
+ access public fields and methods add-exports flags should be added to
container
+ localizers regardless of the user specified java opts. Setting this to
true will
Review Comment:
Nit : JAVA_OPTS
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/ContainerLocalizer.java:
##########
@@ -400,6 +403,13 @@ private LocalizerStatus createStatus() throws
InterruptedException {
public static List<String> getJavaOpts(Configuration conf) {
String opts =
conf.get(YarnConfiguration.NM_CONTAINER_LOCALIZER_JAVA_OPTS_KEY,
YarnConfiguration.NM_CONTAINER_LOCALIZER_JAVA_OPTS_DEFAULT);
+ boolean isExtraJDK17OptionsConfigured =
+
conf.getBoolean(YarnConfiguration.NM_CONTAINER_LOCALIZER_JAVA_OPTS_ADD_EXPORTS_KEY,
+
YarnConfiguration.NM_CONTAINER_LOCALIZER_JAVA_OPTS_ADD_EXPORTS_DEFAULT);
+
Review Comment:
Can you add a testcase for ContainerLocalizer as well?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
