okumin commented on code in PR #5809:
URL: https://github.com/apache/hive/pull/5809#discussion_r2084645866
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java:
##########
@@ -548,8 +550,9 @@ private void setupTezParamsBasedOnMR(TezConfiguration conf)
{
conf.setIfUnset(TezConfiguration.TEZ_AM_LAUNCH_ENV, env);
}
- conf.setIfUnset(TezConfiguration.TEZ_AM_LAUNCH_CMD_OPTS,
- org.apache.tez.mapreduce.hadoop.MRHelpers.getJavaOptsForMRAM(conf));
+ String mrAmJavaOpts =
StringUtils.defaultString(org.apache.tez.mapreduce.hadoop.MRHelpers.getJavaOptsForMRAM(conf));
+ mrAmJavaOpts = String.join(" ", mrAmJavaOpts,
JavaVersionUtils.getAddOpensFlagsIfNeeded());
+ conf.setIfUnset(TezConfiguration.TEZ_AM_LAUNCH_CMD_OPTS, mrAmJavaOpts);
Review Comment:
I am feeling this might not work when TEZ_AM_LAUNCH_CMD_OPTS is explicitly
set. The decision table is like this? I guess we don't update the third pattern.
| JDK | TEZ_AM_LAUNCH_CMD_OPTS is set | Result |
|-|-|-|
| 8 | Yes | Value of TEZ_AM_LAUNCH_CMD_OPTS |
| 8 | No | Value of getJavaOptsForMRAM |
| 17 | Yes | Value of TEZ_AM_LAUNCH_CMD_OPTS + getAddOpensFlagsIfNeeded |
| 17 | No | Value of getJavaOptsForMRAM + getAddOpensFlagsIfNeeded |
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java:
##########
@@ -489,6 +491,15 @@ public int execute() {
return (returnVal);
}
+ private void addJavaOpts(JobConf job) {
+ String commonAddOpens = JavaVersionUtils.getAddOpensFlagsIfNeeded();
+ if(!commonAddOpens.isEmpty()){
+ job.set("yarn.app.mapreduce.am.command-opts", commonAddOpens);
+ job.set("mapreduce.map.java.opts", commonAddOpens);
+ job.set("mapreduce.reduce.java.opts", commonAddOpens);
Review Comment:
Probably, when we add getAddOpensFlagsIfNeeded to `xxx.yyy`, it needs to be
like this.
```
String original = job.get("xxx.yyy", "");
job.set("xxx.yyy", String.join(" ", original, commonAddOpens).trim());
```
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/DagUtils.java:
##########
@@ -708,7 +709,8 @@ Map<String, String> getContainerEnvironment(Configuration
conf, boolean isMap) {
* are set
*/
private static String getContainerJavaOpts(Configuration conf) {
- String javaOpts = HiveConf.getVar(conf,
HiveConf.ConfVars.HIVE_TEZ_JAVA_OPTS);
+ String javaOpts = StringUtils.defaultString(HiveConf.getVar(conf,
HiveConf.ConfVars.HIVE_TEZ_JAVA_OPTS));
+ javaOpts = String.join(" ", javaOpts,
JavaVersionUtils.getAddOpensFlagsIfNeeded());
Review Comment:
I found this disables the warning from L728. Probably, we should update not
`javaOpts` but `finalOpts`.
```
// Probablly, better to trim it because some later steps may do empty-checks
finalOpts = String.join(" ", finalOpts,
JavaVersionUtils.getAddOpensFlagsIfNeeded()).trim();
LOG.debug("Tez container final opts: {}", finalOpts);
return finalOpts;
```
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/tez/TezSessionState.java:
##########
@@ -548,8 +550,9 @@ private void setupTezParamsBasedOnMR(TezConfiguration conf)
{
conf.setIfUnset(TezConfiguration.TEZ_AM_LAUNCH_ENV, env);
}
- conf.setIfUnset(TezConfiguration.TEZ_AM_LAUNCH_CMD_OPTS,
- org.apache.tez.mapreduce.hadoop.MRHelpers.getJavaOptsForMRAM(conf));
+ String mrAmJavaOpts =
StringUtils.defaultString(org.apache.tez.mapreduce.hadoop.MRHelpers.getJavaOptsForMRAM(conf));
+ mrAmJavaOpts = String.join(" ", mrAmJavaOpts,
JavaVersionUtils.getAddOpensFlagsIfNeeded());
+ conf.setIfUnset(TezConfiguration.TEZ_AM_LAUNCH_CMD_OPTS, mrAmJavaOpts);
Review Comment:
I'd write like this.
```
String opts = conf.get(TezConfiguration.TEZ_AM_LAUNCH_CMD_OPTS);
if (opts == null) {
// The MR-related param is a fallback
opts =
StringUtils.defaultString(org.apache.tez.mapreduce.hadoop.MRHelpers.getJavaOptsForMRAM(conf));
}
// Add additional flags if it's JDK 17. Probably, better to trim because
someone does an empty-check
opts = String.join(" ", opts,
JavaVersionUtils.getAddOpensFlagsIfNeeded()).trim();
conf.set(TezConfiguration.TEZ_AM_LAUNCH_CMD_OPTS, opts);
```
##########
hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonControllerJob.java:
##########
@@ -121,10 +123,9 @@ public int run(String[] args) throws IOException,
InterruptedException, ClassNot
if (amMemoryMB != null && !amMemoryMB.isEmpty()) {
conf.set(AppConfig.HADOOP_MR_AM_MEMORY_MB, amMemoryMB);
}
- String amJavaOpts = appConf.controllerAMChildOpts();
- if (amJavaOpts != null && !amJavaOpts.isEmpty()) {
- conf.set(AppConfig.HADOOP_MR_AM_JAVA_OPTS, amJavaOpts);
- }
+ String amJavaOpts =
StringUtils.defaultString(appConf.controllerAMChildOpts());
+ amJavaOpts = String.join(" ", amJavaOpts,
JavaVersionUtils.getAddOpensFlagsIfNeeded());
+ conf.set(AppConfig.HADOOP_MR_AM_JAVA_OPTS, amJavaOpts);
Review Comment:
`AppConfig.HADOOP_MR_AM_JAVA_OPTS` is `yarn.app.mapreduce.am.command-opts`,
and `yarn.app.mapreduce.am.command-opts` has the default value, `-Xmx1024m`. I
guess the default value can be gone.
##########
ql/src/java/org/apache/hadoop/hive/ql/exec/mr/ExecDriver.java:
##########
@@ -489,6 +491,15 @@ public int execute() {
return (returnVal);
}
+ private void addJavaOpts(JobConf job) {
+ String commonAddOpens = JavaVersionUtils.getAddOpensFlagsIfNeeded();
+ if(!commonAddOpens.isEmpty()){
+ job.set("yarn.app.mapreduce.am.command-opts", commonAddOpens);
+ job.set("mapreduce.map.java.opts", commonAddOpens);
+ job.set("mapreduce.reduce.java.opts", commonAddOpens);
Review Comment:
I could be wrong. Is it possible that this overwrites the originally
configured value?
For example, `yarn.app.mapreduce.am.command-opts` is probably configured as
below now.
| JDK | yarn-site.xml | Result | Note |
|-|-|-|-|
| 8 | N/A | -Xmx1024m | Default value |
| 8 | -Xmx2048m | -Xmx2048m | |
With this patch, the table changes as follows.
| JDK | yarn-site.xml | Result | Note |
|-|-|-|-|
| 8 | N/A | -Xmx1024m | Default value |
| 8 | -Xmx2048m | -Xmx2048m | |
| 17 | N/A | -XX:IgnoreUnrecognizedVMOptions --add-opens=... | |
| 17 | -Xmx2048m | -XX:IgnoreUnrecognizedVMOptions --add-opens=... | |
We may be able to point similar things out for other files, such as Tez
configurations.
--
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: gitbox-unsubscr...@hive.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For additional commands, e-mail: gitbox-h...@hive.apache.org
