-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12694467/HADOOP-11509.1.patch
against trunk revision 7b82c4a.

+1 @author. The patch does not contain any @author tags.

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5478//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5478//console

This message is automatically generated.

Patch looks good overall,
one other thing noticed is that:
if user first calls Path.getFileSystem(Configuration conf), user gets an filesystem object;
The next time user invokes Path.getFileSystem(Configuration conf) again with a different conf, the returned filesystem object still uses the old conf.

Should the API Path.getFileSystem(Configuration conf) be that the returned file system object always apply the up-to-date conf ? Chris Nauroth, your opinion ?

Thank you, Xuan and Jian.

Just to provide a bit more background on this, Xuan found that streaming jobs using files in Azure Storage were not able to override the setting of fs.azure.block.size from the command line. It looks like he found the root cause is that validateFiles checks for existence of files against a FileSystem instance, but this FileSystem instance is obtained before handling -D options. This would mean we then have an instance sitting in the FileSystem cache that was created without the -D options set in the Configuration. Later, during MapReduce job split calculation, it would use the cached instance that didn't have the override of fs.azure.block.size.

I agree with the change here, because the expectation is that the command line arguments take precedence. However, I don't think we should move the -D handling all the way to the top of the method. Right now, the handling is such that -D options would take precedence over -fs and -jt. The current patch would reverse that. I don't know if anyone depends on that behavior, but we can avoid changing it by doing the -D handling in between the handling of -conf and the handling of -libjars. I'd be +1 for the patch with that change if you test it and it still works for overriding fs.azure.block.size.

Should the API Path.getFileSystem(Configuration conf) be that the returned file system object always apply the up-to-date conf ?

This is a long-standing weakness of the FileSystem cache. It has been discussed in other jiras, but I can't find those now. The FileSystem cache key is composed of scheme, authority, and UserGroupInformation. However, the FileSystem#get API is phrased in terms of a whole Configuration. Various other configuration properties can tune the behavior of a FileSystem, but if you get a cached instance, then these configuration properties might not be applied. OTOH, it would be too costly to make the whole Configuration part of the cache key.

This is an existing problem, unrelated to the current patch.

Thanks for the comment. Move the -D handling in between the handling of -conf and the handling of -libjars

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12694613/HADOOP-11509.2.patch
against trunk revision 21d5599.

+1 @author. The patch does not contain any @author tags.

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

-1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common:

org.apache.hadoop.security.token.delegation.web.TestWebDelegationToken

Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5484//testReport/
Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5484//console

This message is automatically generated.

+1 for the patch. Thank you again, Xuan. The test failure is unrelated. I verified that it passes locally for me.

Thanks, Chris Nauroth and Jian He
I have tested the patch locally. It works for overriding fs.azure.block.size.

Committed to trunk/branch-2. Thanks for the review.

FAILURE: Integrated in Hadoop-trunk-Commit #6936 (See https://builds.apache.org/job/Hadoop-trunk-Commit/6936/)
HADOOP-11509. Change parsing sequence in GenericOptionsParser to parse (xgong: rev 0bf333911c950f22ec0f784bf465306e20b0d507)

• hadoop-common-project/hadoop-common/CHANGES.txt
• hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/GenericOptionsParser.java

Umm, isn't this an incompatible change? I'm sort of surprised to see this committed to branch-2.

Hi Allen Wittenauer. Based on my code review, I thought this change was OK for backwards compatibility.

The first version would have been backwards-incompatible, because it would have changed the precedence of -D arguments over -fs and -jt. (See 2 comments back from me for more details.)

The committed version of the patch still moves the -D handling before the handling of -libjars, -files and -archives. However, these are not analogous to -fs and -jt. They do not map directly to an underlying user-facing configuration property, so I expect the change in precedence isn't relevant. While it is a change in behavior, I think the prior behavior can only be considered a bug due to validateFiles caching a FileSystem with configuration that didn't consider the -D arguments.

Please let me know if you disagree or if you think I missed something in the review. The intent was not to put a backwards-incompatible change in branch-2.

I understand your position and mostly agree. But we have a huge documentation problem: all of the examples and java help messages, etc, for the entirety of Hadoop's lifetime have these options in a different order. Since it doesn't appear that the parser is particularly smart about processing these things in any particular order, all of these examples break. This likely means real world code breaks.

At a minimum, I think this patch requires some human-side edits to make sure everything is in sync with the expected order of the options.

FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #86 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/86/)
HADOOP-11509. Change parsing sequence in GenericOptionsParser to parse (xgong: rev 0bf333911c950f22ec0f784bf465306e20b0d507)

• hadoop-common-project/hadoop-common/CHANGES.txt
• hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/GenericOptionsParser.java

FAILURE: Integrated in Hadoop-Yarn-trunk #820 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/820/)
HADOOP-11509. Change parsing sequence in GenericOptionsParser to parse (xgong: rev 0bf333911c950f22ec0f784bf465306e20b0d507)

• hadoop-common-project/hadoop-common/CHANGES.txt
• hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/GenericOptionsParser.java

FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #83 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/83/)
HADOOP-11509. Change parsing sequence in GenericOptionsParser to parse (xgong: rev 0bf333911c950f22ec0f784bf465306e20b0d507)

• hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/GenericOptionsParser.java
• hadoop-common-project/hadoop-common/CHANGES.txt

FAILURE: Integrated in Hadoop-Mapreduce-trunk #2037 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2037/)
HADOOP-11509. Change parsing sequence in GenericOptionsParser to parse (xgong: rev 0bf333911c950f22ec0f784bf465306e20b0d507)

• hadoop-common-project/hadoop-common/CHANGES.txt
• hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/GenericOptionsParser.java

SUCCESS: Integrated in Hadoop-Hdfs-trunk #2018 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2018/)
HADOOP-11509. Change parsing sequence in GenericOptionsParser to parse (xgong: rev 0bf333911c950f22ec0f784bf465306e20b0d507)

• hadoop-common-project/hadoop-common/CHANGES.txt
• hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/GenericOptionsParser.java

FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #87 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/87/)
HADOOP-11509. Change parsing sequence in GenericOptionsParser to parse (xgong: rev 0bf333911c950f22ec0f784bf465306e20b0d507)

• hadoop-common-project/hadoop-common/CHANGES.txt
• hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/GenericOptionsParser.java

I took a look at how the generic command line options are documented, currently and going back to 1.0.4:

http://hadoop.apache.org/docs/r2.6.0/hadoop-project-dist/hadoop-common/CommandsManual.html#Generic_Options

http://hadoop.apache.org/docs/r1.0.4/commands_manual.html#Generic+Options

Also relevant is the streaming documentation, which basically repeats the information:

http://hadoop.apache.org/docs/r2.6.0/hadoop-mapreduce-client/hadoop-mapreduce-client-core/HadoopStreaming.html#Generic_Command_Options

http://hadoop.apache.org/docs/r1.0.4/streaming.html#Generic+Command+Options

There is no way for a user to interpret this documentation to get a complete and correct understanding of these precedence rules (regardless of this patch). It sounds like you're suggesting we file a follow-on jira to improve the documentation. Do I understand correctly?

I still can't see a way that this patch would cause calling code to break. The argument handling at this layer is not strictly positional, due to the way commons-cli works. I don't expect anyone will need to swap the order of arguments in their script or anything like that. Do you have an example of something that would break after this patch?

FWIW, my experience with the YARN-679 "generic entry point" code was that any attempt to instantiate a FileSystem instance, even indirectly, would trigger the UGI setup. If HdfsConfiguration and, if needed YarnConfiguration hadn't been instantiated first, you'd have an invalid filesystem/auth combo.

it was that, not the order of -D eval on the CLI, which caused problems

Sometimes you can work around these problems by configuring fs.<file system scheme>.impl.disable.cache to true or using FileSystem#newInstance to guarantee a certain spot in the code gets a distinct instance that won't be pulled from the cache later by another part of the code. However, this has its own problems. Disabling the cache can cause a performance problem. If using FileSystem#newInstance, then it's very important that the owner of the instance eventually call close to avoid bloating memory over time. In the case where the FileSystem instance creation happens indirectly, these workarounds might not be viable, because you might not want to change code several layers below just to accommodate a new patch.

Issues like this have made the FileSystem cache a frequent source of confusion. This might warrant a redesign at some point. I'd prefer that a lot of the implicit behavior around use of Configuration and UserGroupInformation were made explicit to the caller in the API. I also think we'd benefit from full reference counting instead of the current "best effort" caching where an unrelated thread could call close and trigger a cache eviction. I believe doing this would require changing the contract so that callers must always call close. Unfortunately, that would be backwards-incompatible. There is a ton of existing code all over the ecosystem that doesn't bother calling close.

UGI and Config setup are irreversible. Once UGI has set itself up, you are stuck with the singleton. As for Configurations, give it an invalid XML document as a base resource and there goes Hadoop

I have filed HADOOP-11528 to improve the documentation to discuss the precedence rules for generic command line options.

Has anyone found a concrete example of a backwards-compatibility problem, or has this discussion been addressed adequately? Please comment if there are still concerns. Otherwise, I'm going to move on from this jira.

Thanks!

Sorry, I haven't had a chance to try things where I knew precedence mattered. Let's move on and just wait to see what breaks.

Thanks.

Thank you for reviewing, Allen Wittenauer.