[ 
https://issues.apache.org/jira/browse/TEZ-3874?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16383816#comment-16383816
 ] 

Jason Lowe commented on TEZ-3874:
---------------------------------

Thanks for updating the patch! Here's the QA report:
----
{color:#FF0000}-1 overall{color}. Here are the results of testing the latest 
attachment
 [http://issues.apache.org/jira/secure/attachment/12912688/TEZ-3874.3.patch]
 against master revision bb40cf5.

{color:#008000}+1 @author{color}. The patch does not contain any @author tags.

{color:#008000}+1 tests included{color}. The patch appears to include 2 new or 
modified test files.

{color:#008000}+1 javac{color}. The applied patch does not increase the total 
number of javac compiler warnings.

{color:#008000}+1 javadoc{color}. There were no new javadoc warning messages.

{color:#008000}+1 findbugs{color}. The patch does not introduce any new 
Findbugs (version 3.0.1) warnings.

{color:#008000}+1 release audit{color}. The applied patch does not increase the 
total number of release audit warnings.

{color:#FF0000}-1 core tests{color}. The patch failed these unit tests in :
 org.apache.tez.analyzer.TestAnalyzer

Test results: 
[https://builds.apache.org/job/PreCommit-TEZ-Build/2732//testReport/]
 Console output: 
[https://builds.apache.org/job/PreCommit-TEZ-Build/2732//console]
----
Again I'm not sure the notNullKvp method is worth the weight. I do not see the 
utility of a Precondition not null check vs. just letting it NPE on the line 
number where it is used. Both are going to throw an NPE and it will be obvious 
which is null for that line. And once we remove the Precondition null checks, 
the function boils down to:
{code:java}
  public static boolean notNullKvpWithValueReplacement(Map.Entry<String, 
String> kvp, Configuration conf) {
    String key = kvp.getKey();
    String value = conf.get(key);
    return value != null;
  }
{code}
which IMHO is just not worth it. For example:
{code:java}
      if(TezUtils.notNullKvpWithValueReplacement(entry, amConf)) {
        PlanKeyValuePair.Builder kvp = PlanKeyValuePair.newBuilder();
        kvp.setKey(entry.getKey());
        kvp.setValue(amConf.get(entry.getKey()));
{code}
is equivalent to:
{code:java}
      String val = amConf.get(entry.getKey());
      if (val != null) {
        PlanKeyValuePair.Builder kvp = PlanKeyValuePair.newBuilder();
        kvp.setKey(entry.getKey());
        kvp.setValue(val);
{code}
which is simpler to read and understand what's going on. As a bonus it avoids 
the double-lookup on the conf key in the common case where the value is not 
null.

There's no need for the debug check before doing the debug logs. One main 
benefit of SLF4J's API vs. log4j is avoiding all the explicit log level checks 
if all we're doing is passing positional parameters for the message that don't 
need to be computed just for the log.

I don't think it's worth exposing a public static for the debug log message. 
It's reaching cross-module and exposes potential reference outside of Tez.

> NPE in TezClientUtils when "yarn.resourcemanager.zk-address" is present in 
> Configuration
> ----------------------------------------------------------------------------------------
>
>                 Key: TEZ-3874
>                 URL: https://issues.apache.org/jira/browse/TEZ-3874
>             Project: Apache Tez
>          Issue Type: Bug
>    Affects Versions: 0.9.1
>            Reporter: Eric Wohlstadter
>            Assignee: Eric Wohlstadter
>            Priority: Blocker
>         Attachments: TEZ-3874.1.patch, TEZ-3874.3.patch, TEZ-3874.patch.2
>
>   Original Estimate: 48h
>  Remaining Estimate: 48h
>
> "yarn.resourcemanager.zk-address" is deprecated in favor of 
> "hadoop.zk.address" for Hadoop 2.9+.
> Configuration base class does't auto-translate the deprecation. Only 
> YarnConfiguration applies the translation.
> In TezClientUtils.createFinalConfProtoForApp, a NPE is throw if 
> "yarn.resourcemanager.zk-address" is present in the Configuration.
> {code}
> for (Entry<String, String> entry : amConf) {
>       PlanKeyValuePair.Builder kvp = PlanKeyValuePair.newBuilder();
>       kvp.setKey(entry.getKey());
>       kvp.setValue(amConf.get(entry.getKey()));
>       builder.addConfKeyValues(kvp);
>     }
> {code}
> Even though Tez is not specifically looking for the deprecated property, 
> {{amConf.get(entry.getKey())}} will find it during the iteration, if it is in 
> any of the merged xml property resources. 
> {{amConf.get(entry.getKey())}} will return null, and {{kvp.setValue(null)}} 
> will trigger NPE.
> Suggested solution is to change to: 
> {code}
> YarnConfiguration wrappedConf = new YarnConfiguration(amConf);
> for (Entry<String, String> entry : wrappedConf) {
>       PlanKeyValuePair.Builder kvp = PlanKeyValuePair.newBuilder();
>       kvp.setKey(entry.getKey());
>       kvp.setValue(wrappedConf.get(entry.getKey()));
>       builder.addConfKeyValues(kvp);
>     }
> {code}



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Reply via email to