tomicooler commented on code in PR #5935:
URL: https://github.com/apache/hadoop/pull/5935#discussion_r1289663461
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/launcher/ContainerLaunch.java:
##########
@@ -170,6 +170,13 @@ public static String expandEnvironment(String var,
var = var.replace(ApplicationConstants.CLASS_PATH_SEPARATOR,
File.pathSeparator);
+ if (Shell.isJavaVersionAtLeast(17)) {
Review Comment:
Just wondering about this. Wouldn't a node-manager config be easer to reason
about?
Implementing like this is a bit strange, currently MR jobs can be configured
(default=true) and the distributed shell app sets this regardless of the
configuration. During the job submission/config time it's not clear if
container will be launched on a Java>=17 node so that's the reason for the
placeholder, later ContainerLaunch replaces it to the arg or an empty string.
Maybe non MR apps would also require this option to run properly - which they
could specify at the job config - but non-homogenous nodes (where nodes have
different Java installs) can't be handled easily (maybe with node labels or
some other trick).
I think this should be a node-manager config instead. The ContainerLaunch
could just simply add the arg when the java version is at least 17 and the
config option is set (if we can detect that the container is a java app, not
sure about that).
--
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: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
