[
https://issues.apache.org/jira/browse/TEZ-3874?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16338065#comment-16338065
]
Jason Lowe commented on TEZ-3874:
---------------------------------
Thanks for updating the patch! The precommit build is having trouble posting
to JIRA, but here's what it tried to say:
{color:red}-1 overall{color}. Here are the results of testing the latest
attachment
http://issues.apache.org/jira/secure/attachment/12907433/TEZ-3874.patch.2
against master revision 3c7640d.
{color:green}+1 @author{color}. The patch does not contain any @author
tags.
{color:green}+1 tests included{color}. The patch appears to include 3 new
or modified test files.
{color:green}+1 javac{color}. The applied patch does not increase the
total number of javac compiler warnings.
{color:green}+1 javadoc{color}. There were no new javadoc warning messages.
{color:red}-1 findbugs{color}. The patch appears to introduce 2 new
Findbugs (version 3.0.1) warnings.
{color:green}+1 release audit{color}. The applied patch does not increase
the total number of release audit warnings.
{color:green}+1 core tests{color}. The patch passed unit tests in .
Test results:
https://builds.apache.org/job/PreCommit-TEZ-Build/2720//testReport/
Findbugs warnings:
https://builds.apache.org/job/PreCommit-TEZ-Build/2720//artifact/patchprocess/newPatchFindbugsWarningstez-api.html
Console output: https://builds.apache.org/job/PreCommit-TEZ-Build/2720//console
The findbug warnings are about variables that are apparently constants but are
not marked as such. Other comments on the patch:
TezClientUtils changed to use wildcard imports, and last I checked we preferred
to not use wildcard importing.
I'm not sure we want to log a warning or even an info when this occurs. As I
mentioned above, null key values can do happen in practice, and it isn't
necessarily an error when they do. I would either remove the log messages or
make them debug at best. Otherwise I can see this spamming the user's logs
with stuff they may not be able to fix themselves.
Nit: There's a lot of places in the code that does essentially this:
{code}
for (Entry<String, String> entry : amConf) {
if(TezUtils.notNullKvpWithValueReplacement(entry, amConf)) {
PlanKeyValuePair.Builder kvp = PlanKeyValuePair.newBuilder();
kvp.setKey(entry.getKey());
kvp.setValue(amConf.get(entry.getKey()));
builder.addConfKeyValues(kvp);
} else {
LOG.warn(NULL_KVP_REPLACEMENT_LOG_MSG, entry.getKey());
}
}
{code}
and it would be good to factor out that as a utility method.
The test should not catch all Exceptions and suppress them. If the test causes
some other exception to be thrown, like IllegalArgumentException, then
suppressing it is not desired. If we're testing to make sure an NPE isn't
thrown then all we should do is call it _without catching_. The test framework
will ensure the test is marked as failed if NPE or any other exception gets
thrown out from the test method.
Nit: IMHO the new notNullKvp methods are a bit overkill. Most of the
Precondition checks are unnecessary given where it's used, and this essentially
boils down to a null check after we fetch a value. Not a must-fix, just an
opinion. Once all the common conf-copying code is factored out, the need for
these utility functions diminishes further.
> 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
> Priority: Blocker
> Attachments: TEZ-3874.1.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)
