Steve Loughran, 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.
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.
TestShell.testGetCheckProcessIsAliveCommand:203->Assert.assertArrayEquals:280->Assert.assertArrayEquals:265->Assert.internalArrayEquals:473 arrays first differed at element ; 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 ; expected:<...oject\hadoop-common\[..\..\hadoop-common-project\hadoop-common\]target\bin\winutils....> but was:<...oject\hadoop-common\target\bin\winutils....>
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
- On Shell#WINDOWS_MAX_SHELL_LENGHT, JavaDoc is being picky and giving a warning about @deprecated:. According to JavaDoc documentation, 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.
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.