Uploaded image for project: 'Hadoop Common'
  1. Hadoop Common
  2. HADOOP-11509

change parsing sequence in GenericOptionsParser to parse -D parameters first

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.7.0
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      In GenericOptionsParser, we need to parse -D parameter first. In that case, the user input parameter (through -D) can be set into configuration object earlier and used to process other parameters.

      1. HADOOP-11509.2.patch
        2 kB
        Xuan Gong
      2. HADOOP-11509.1.patch
        2 kB
        Xuan Gong

        Issue Links

          Activity

          Hide
          hadoopqa Hadoop QA added a comment -

          -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.

          Show
          hadoopqa Hadoop QA added a comment - -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.
          Hide
          jianhe Jian He added a comment -

          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 ?

          Show
          jianhe Jian He added a comment - 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 ?
          Hide
          cnauroth Chris Nauroth added a comment -

          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.

          Show
          cnauroth Chris Nauroth added a comment - 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.
          Hide
          xgong Xuan Gong added a comment -

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

          Show
          xgong Xuan Gong added a comment - Thanks for the comment. Move the -D handling in between the handling of -conf and the handling of -libjars
          Hide
          hadoopqa Hadoop QA added a comment -

          -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.

          Show
          hadoopqa Hadoop QA added a comment - -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.
          Hide
          cnauroth Chris Nauroth added a comment - - edited

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

          Show
          cnauroth Chris Nauroth added a comment - - edited +1 for the patch. Thank you again, Xuan. The test failure is unrelated. I verified that it passes locally for me.
          Hide
          xgong Xuan Gong added a comment -

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

          Show
          xgong Xuan Gong added a comment - Thanks, Chris Nauroth and Jian He I have tested the patch locally. It works for overriding fs.azure.block.size.
          Hide
          xgong Xuan Gong added a comment -

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

          Show
          xgong Xuan Gong added a comment - Committed to trunk/branch-2. Thanks for the review.
          Hide
          hudson Hudson added a comment -

          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
          Show
          hudson Hudson added a comment - 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
          Hide
          aw Allen Wittenauer added a comment -

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

          Show
          aw Allen Wittenauer added a comment - Umm, isn't this an incompatible change? I'm sort of surprised to see this committed to branch-2.
          Hide
          cnauroth Chris Nauroth added a comment -

          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.

          Show
          cnauroth Chris Nauroth added a comment - 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.
          Hide
          aw Allen Wittenauer added a comment -

          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.

          Show
          aw Allen Wittenauer added a comment - 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.
          Hide
          hudson Hudson added a comment -

          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
          Show
          hudson Hudson added a comment - 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
          Hide
          hudson Hudson added a comment -

          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
          Show
          hudson Hudson added a comment - 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
          Hide
          hudson Hudson added a comment -

          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
          Show
          hudson Hudson added a comment - 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
          Hide
          hudson Hudson added a comment -

          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
          Show
          hudson Hudson added a comment - 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
          Hide
          hudson Hudson added a comment -

          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
          Show
          hudson Hudson added a comment - 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
          Hide
          hudson Hudson added a comment -

          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
          Show
          hudson Hudson added a comment - 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
          Hide
          cnauroth Chris Nauroth added a comment -

          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?

          Show
          cnauroth Chris Nauroth added a comment - 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?
          Hide
          stevel@apache.org Steve Loughran added a comment -

          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

          Show
          stevel@apache.org Steve Loughran added a comment - 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
          Hide
          cnauroth Chris Nauroth added a comment -

          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.

          Show
          cnauroth Chris Nauroth added a comment - 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 .
          Hide
          stevel@apache.org Steve Loughran added a comment -

          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

          Show
          stevel@apache.org Steve Loughran added a comment - 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
          Hide
          cnauroth Chris Nauroth added a comment -

          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!

          Show
          cnauroth Chris Nauroth added a comment - 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!
          Hide
          aw Allen Wittenauer added a comment -

          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.

          Show
          aw Allen Wittenauer added a comment - 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.
          Hide
          cnauroth Chris Nauroth added a comment -

          Thank you for reviewing, Allen Wittenauer.

          Show
          cnauroth Chris Nauroth added a comment - Thank you for reviewing, Allen Wittenauer .

            People

            • Assignee:
              xgong Xuan Gong
              Reporter:
              xgong Xuan Gong
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development