[
https://issues.apache.org/jira/browse/HADOOP-10775?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14942821#comment-14942821
]
Chris Nauroth commented on HADOOP-10775:
----------------------------------------
[~ste...@apache.org], this is a great clean-up, and the extra test coverage is
good too.
# Please expand the comment on {{Shell#isJava7OrAbove}} to describe why it's
hard-coded to return {{true}}. Something like "Hadoop starting at version X
requires Java 7 or above" would help clarify.
# bq. why was the {{AtomicBoolean}} {{completed}} marked as {{volatile?}}
It seems this was trying to compensate for the fact that the {{AtomicBoolean}}
was instantiated lazily, and then could have been referenced on a different
thread. The methods of {{AtomicBoolean}} have ordering guarantees, but there
wouldn't have been an ordering guarantee on the reference to the
{{AtomicBoolean}}. I agree with the change made in your patch to guarantee
initialization at {{Shell}} construction instead. It's easier to understand.
# I saw test failures in {{TestShell#testGetCheckProcessIsAliveCommand}},
{{TestShell#testGetSignalKillCommand}} and
{{TestContainerLaunch#testWindowsShellScriptBuilderLink}} when running on
Windows. See below. For {{TestShell}}, it looks like we'll need to
canonicalize the expected paths. I didn't dig deep into the
{{TestContainerLaunch}} failure yet, but it appears to be connected to the max
command line length checks.
{code}
Failed tests:
TestShell.testGetCheckProcessIsAliveCommand:203->Assert.assertArrayEquals:280->Assert.assertArrayEquals:265->Assert.internalArrayEquals:473
arrays first differed at element [0];
expected:<...oject\hadoop-common\[..\..\hadoop-common-project\hadoop-common\]target\bin\winutils....>
but was:<...oject\hadoop-common\[]target\bin\winutils....>
TestShell.testGetSignalKillCommand:223->Assert.assertArrayEquals:280->Assert.assertArrayEquals:265->Assert.internalArrayEquals:473
arrays first differed at element [0];
expected:<...oject\hadoop-common\[..\..\hadoop-common-project\hadoop-common\]target\bin\winutils....>
but was:<...oject\hadoop-common\[]target\bin\winutils....>
{code}
{code}
testWindowsShellScriptBuilderLink(org.apache.hadoop.yarn.server.nodemanager.containermanager.launcher.TestContainerLaunch)
Time elapsed: 0.078 sec <<< FAILURE!
java.lang.AssertionError: long link was expected to throw
at org.junit.Assert.fail(Assert.java:88)
at
org.apache.hadoop.yarn.server.nodemanager.containermanager.launcher.TestContainerLaunch.testWindowsShellScriptBuilderLink(TestContainerLaunch.java:1096)
{code}
# On {{Shell#WINDOWS_MAX_SHELL_LENGHT}}, JavaDoc is being picky and giving a
warning about {{@deprecated:}}. According to [JavaDoc
documentation|http://docs.oracle.com/javase/1.5.0/docs/guide/javadoc/deprecation/deprecation.html#javadoc_tag],
the tag must be followed immediately by a space or a newline, so it doesn't
like the colon.
# There is one more checkstyle warning to fix.
Test failures in the last Jenkins run were unrelated.
bq. The change in {{getWinutilsPath()}} is something which could be flagged as
incompatible. There's nothing in the Hadoop codebase which appears to break
with this change, all that is happing is that NPEs are being replaced with
useful text.
IIUC, the concern is that an application previously could have caught the
{{NullPointerException}} and attempted some kind of fallback option. After
this patch, it becomes a {{RuntimeException}}, so that error handler wouldn't
get triggered. I think it would be unusual for this to be a problem in
practice, but if we really want to be conservative, then we could continue
throwing {{NullPointerException}}, populated with the new text. Of course, it
isn't really a null pointer problem, but the text would clarify that. Let me
know your thoughts.
> Shell operations to fail with meaningful errors on windows if winutils.exe
> not found
> ------------------------------------------------------------------------------------
>
> Key: HADOOP-10775
> URL: https://issues.apache.org/jira/browse/HADOOP-10775
> Project: Hadoop Common
> Issue Type: Improvement
> Components: util
> Affects Versions: trunk-win, 2.7.1
> Environment: windows
> Reporter: Steve Loughran
> Assignee: Steve Loughran
> Priority: Minor
> Attachments: HADOOP-10775-002.patch, HADOOP-10775-003.patch,
> HADOOP-10775-004.patch, HADOOP-10775-005.patch, HADOOP-10775.patch
>
>
> If {{winutils.exe}} can't be found {{HADOOP_HOME}} wrong/unset or other
> causes, then an error is logged -but when any of the {{Shell}} operations are
> used, an NPE is raised rather than something meaningful.
> The error message at setup time should be preserved and then raised before
> any attempt to invoke a winutils-driven process made
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
