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

StringIndexOutOfBoundsException breaks org.apache.hadoop.util.Shell on 2.7.x with Java 9

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.7.2
    • Fix Version/s: 2.7.4
    • Component/s: common
    • Labels:
    • Environment:

      Java 9, build 175 (Java 9 release candidate as of June 25th, 2017)

    • Target Version/s:
    • Hadoop Flags:
      Reviewed

      Description

      You cannot use any pre-Hadoop 2.8 component anymore with the latest release candidate build of Java 9, because it fails with an StringIndexOutOfBoundsException in org.apache.hadoop.util.Shell#<clinit>. This leads to a whole cascade of failing classes (next in chain is StringUtils).

      The reason is that the release candidate build of Java 9 no longer has "-ea" in the version string and the system property "java.version" is now simply "9". This causes the following line to fail fatally:

        private static boolean IS_JAVA7_OR_ABOVE =
            System.getProperty("java.version").substring(0, 3).compareTo("1.7") >= 0;
      

      Analysis:

      • This code looks wrong, as comparing a version this way is incorrect.
      • The substring(0, 3) is not needed, compareTo also works without it, although it is still an invalid way to compare a version.
      1. HADOOP-14586-branch-2.7-01.patch
        8 kB
        Akira Ajisaka
      2. HADOOP-14586-branch-2.7-02.patch
        0.8 kB
        Uwe Schindler
      3. HADOOP-14586-branch-2.7-03.patch
        0.9 kB
        Konstantin Shvachko

        Issue Links

          Activity

          Hide
          ajisakaa Akira Ajisaka added a comment -

          This code was removed by HADOOP-10775 and it does not fail in 2.8.0+

          Show
          ajisakaa Akira Ajisaka added a comment - This code was removed by HADOOP-10775 and it does not fail in 2.8.0+
          Hide
          thetaphi Uwe Schindler added a comment -

          I am not sure, if we can easily upgrade. Is there any plan to release 2.7.4 with the useless substring() call removed. It is not needed at all.

          Show
          thetaphi Uwe Schindler added a comment - I am not sure, if we can easily upgrade. Is there any plan to release 2.7.4 with the useless substring() call removed. It is not needed at all.
          Hide
          ajisakaa Akira Ajisaka added a comment -

          Thanks. I'm thinking we can easily fix this in 2.7.4.

          Show
          ajisakaa Akira Ajisaka added a comment - Thanks. I'm thinking we can easily fix this in 2.7.4.
          Hide
          hadoopqa Hadoop QA added a comment -
          -1 overall



          Vote Subsystem Runtime Comment
          0 reexec 11m 35s Docker mode activated.
          +1 @author 0m 0s The patch does not contain any @author tags.
          +1 test4tests 0m 0s The patch appears to include 2 new or modified test files.
          0 mvndep 0m 21s Maven dependency ordering for branch
          +1 mvninstall 7m 43s branch-2.7 passed
          +1 compile 5m 12s branch-2.7 passed with JDK v1.8.0_131
          +1 compile 6m 17s branch-2.7 passed with JDK v1.7.0_131
          +1 checkstyle 1m 19s branch-2.7 passed
          +1 mvnsite 1m 12s branch-2.7 passed
          -1 findbugs 1m 29s hadoop-common-project/hadoop-common in branch-2.7 has 3 extant Findbugs warnings.
          +1 javadoc 0m 58s branch-2.7 passed with JDK v1.8.0_131
          +1 javadoc 1m 14s branch-2.7 passed with JDK v1.7.0_131
          0 mvndep 0m 15s Maven dependency ordering for patch
          +1 mvninstall 0m 58s the patch passed
          +1 compile 5m 7s the patch passed with JDK v1.8.0_131
          +1 javac 5m 7s the patch passed
          +1 compile 6m 18s the patch passed with JDK v1.7.0_131
          +1 javac 6m 18s the patch passed
          -0 checkstyle 1m 17s root: The patch generated 1 new + 485 unchanged - 4 fixed = 486 total (was 489)
          +1 mvnsite 1m 12s the patch passed
          -1 whitespace 0m 0s The patch has 2542 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply
          -1 whitespace 1m 57s The patch 128 line(s) with tabs.
          +1 findbugs 2m 40s the patch passed
          +1 javadoc 0m 57s the patch passed with JDK v1.8.0_131
          +1 javadoc 1m 12s the patch passed with JDK v1.7.0_131
          -1 unit 19m 9s hadoop-common in the patch failed with JDK v1.7.0_131.
          +1 unit 6m 33s hadoop-yarn-server-nodemanager in the patch passed with JDK v1.7.0_131.
          +1 asflicense 0m 20s The patch does not generate ASF License warnings.
          112m 53s



          Reason Tests
          JDK v1.8.0_131 Failed junit tests hadoop.util.bloom.TestBloomFilters
          JDK v1.8.0_131 Timed out junit tests org.apache.hadoop.conf.TestConfiguration
          JDK v1.7.0_131 Failed junit tests hadoop.ipc.TestIPC
            hadoop.util.bloom.TestBloomFilters
          JDK v1.7.0_131 Timed out junit tests org.apache.hadoop.conf.TestConfiguration



          Subsystem Report/Notes
          Docker Image:yetus/hadoop:67e87c9
          JIRA Issue HADOOP-14586
          JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12874377/HADOOP-14586-branch-2.7-01.patch
          Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle
          uname Linux 5038dc20bafd 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
          Build tool maven
          Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh
          git revision branch-2.7 / 80ed105
          Default Java 1.7.0_131
          Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_131 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_131
          findbugs v3.0.0
          findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/12621/artifact/patchprocess/branch-findbugs-hadoop-common-project_hadoop-common-warnings.html
          checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12621/artifact/patchprocess/diff-checkstyle-root.txt
          whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/12621/artifact/patchprocess/whitespace-eol.txt
          whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/12621/artifact/patchprocess/whitespace-tabs.txt
          unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12621/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.7.0_131.txt
          JDK v1.7.0_131 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12621/testReport/
          modules C: hadoop-common-project/hadoop-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager U: .
          Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12621/console
          Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org

          This message was automatically generated.

          Show
          hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 reexec 11m 35s Docker mode activated. +1 @author 0m 0s The patch does not contain any @author tags. +1 test4tests 0m 0s The patch appears to include 2 new or modified test files. 0 mvndep 0m 21s Maven dependency ordering for branch +1 mvninstall 7m 43s branch-2.7 passed +1 compile 5m 12s branch-2.7 passed with JDK v1.8.0_131 +1 compile 6m 17s branch-2.7 passed with JDK v1.7.0_131 +1 checkstyle 1m 19s branch-2.7 passed +1 mvnsite 1m 12s branch-2.7 passed -1 findbugs 1m 29s hadoop-common-project/hadoop-common in branch-2.7 has 3 extant Findbugs warnings. +1 javadoc 0m 58s branch-2.7 passed with JDK v1.8.0_131 +1 javadoc 1m 14s branch-2.7 passed with JDK v1.7.0_131 0 mvndep 0m 15s Maven dependency ordering for patch +1 mvninstall 0m 58s the patch passed +1 compile 5m 7s the patch passed with JDK v1.8.0_131 +1 javac 5m 7s the patch passed +1 compile 6m 18s the patch passed with JDK v1.7.0_131 +1 javac 6m 18s the patch passed -0 checkstyle 1m 17s root: The patch generated 1 new + 485 unchanged - 4 fixed = 486 total (was 489) +1 mvnsite 1m 12s the patch passed -1 whitespace 0m 0s The patch has 2542 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply -1 whitespace 1m 57s The patch 128 line(s) with tabs. +1 findbugs 2m 40s the patch passed +1 javadoc 0m 57s the patch passed with JDK v1.8.0_131 +1 javadoc 1m 12s the patch passed with JDK v1.7.0_131 -1 unit 19m 9s hadoop-common in the patch failed with JDK v1.7.0_131. +1 unit 6m 33s hadoop-yarn-server-nodemanager in the patch passed with JDK v1.7.0_131. +1 asflicense 0m 20s The patch does not generate ASF License warnings. 112m 53s Reason Tests JDK v1.8.0_131 Failed junit tests hadoop.util.bloom.TestBloomFilters JDK v1.8.0_131 Timed out junit tests org.apache.hadoop.conf.TestConfiguration JDK v1.7.0_131 Failed junit tests hadoop.ipc.TestIPC   hadoop.util.bloom.TestBloomFilters JDK v1.7.0_131 Timed out junit tests org.apache.hadoop.conf.TestConfiguration Subsystem Report/Notes Docker Image:yetus/hadoop:67e87c9 JIRA Issue HADOOP-14586 JIRA Patch URL https://issues.apache.org/jira/secure/attachment/12874377/HADOOP-14586-branch-2.7-01.patch Optional Tests asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle uname Linux 5038dc20bafd 4.4.0-43-generic #63-Ubuntu SMP Wed Oct 12 13:48:03 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux Build tool maven Personality /testptch/hadoop/patchprocess/precommit/personality/provided.sh git revision branch-2.7 / 80ed105 Default Java 1.7.0_131 Multi-JDK versions /usr/lib/jvm/java-8-oracle:1.8.0_131 /usr/lib/jvm/java-7-openjdk-amd64:1.7.0_131 findbugs v3.0.0 findbugs https://builds.apache.org/job/PreCommit-HADOOP-Build/12621/artifact/patchprocess/branch-findbugs-hadoop-common-project_hadoop-common-warnings.html checkstyle https://builds.apache.org/job/PreCommit-HADOOP-Build/12621/artifact/patchprocess/diff-checkstyle-root.txt whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/12621/artifact/patchprocess/whitespace-eol.txt whitespace https://builds.apache.org/job/PreCommit-HADOOP-Build/12621/artifact/patchprocess/whitespace-tabs.txt unit https://builds.apache.org/job/PreCommit-HADOOP-Build/12621/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common-jdk1.7.0_131.txt JDK v1.7.0_131 Test Results https://builds.apache.org/job/PreCommit-HADOOP-Build/12621/testReport/ modules C: hadoop-common-project/hadoop-common hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager U: . Console output https://builds.apache.org/job/PreCommit-HADOOP-Build/12621/console Powered by Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org This message was automatically generated.
          Hide
          thetaphi Uwe Schindler added a comment -

          Thanks Akira Ajisaka! I am currently trying to update Apache Solr to 2.8.1, I got it compiling, but I am not sure about other dependencies.

          Show
          thetaphi Uwe Schindler added a comment - Thanks Akira Ajisaka ! I am currently trying to update Apache Solr to 2.8.1, I got it compiling, but I am not sure about other dependencies.
          Hide
          thetaphi Uwe Schindler added a comment -

          We are currently unable to upgrade to 2.8.1, because the new MiniDFSCluster breaks our test framework. It keeps threads that are not even interruptible lingering around. This cannot be in Solr's code, so we have to stick with 2.7.x and therefore the fix is really required (especially for Solr 6.x, which is oficially "Java 9 compatible"). We don't like to fork Hadoop-Common and ship with a patched version, so this fix is really urgent.

          Show
          thetaphi Uwe Schindler added a comment - We are currently unable to upgrade to 2.8.1, because the new MiniDFSCluster breaks our test framework. It keeps threads that are not even interruptible lingering around. This cannot be in Solr's code, so we have to stick with 2.7.x and therefore the fix is really required (especially for Solr 6.x, which is oficially "Java 9 compatible"). We don't like to fork Hadoop-Common and ship with a patched version, so this fix is really urgent.
          Hide
          alanb Alan Bateman added a comment -

          JEP 223 (http://openjdk.java.net/jeps/223) details the format of the version string, including the java.version property.

          As Uwe reports, this code fragment in Hadoop works almost by accident with "9-ea". The "ea" component is the pre-release identifier (or $PRE) in JEP 223 and this has now been dropped because JDK 9 is close to its first GA candidate build.

          Show
          alanb Alan Bateman added a comment - JEP 223 ( http://openjdk.java.net/jeps/223 ) details the format of the version string, including the java.version property. As Uwe reports, this code fragment in Hadoop works almost by accident with "9-ea". The "ea" component is the pre-release identifier (or $PRE) in JEP 223 and this has now been dropped because JDK 9 is close to its first GA candidate build.
          Hide
          thetaphi Uwe Schindler added a comment -

          Thanks Alan!
          The interesting thing - as far as I understood - the whole code is outdated and no longer applicable, as Java 7 is already minimum requirement for Hadoop 2.7.x. So such buggy code parts still lingering around is a big issue in open source projects like hadoop, especially in static initializers.

          Show
          thetaphi Uwe Schindler added a comment - Thanks Alan! The interesting thing - as far as I understood - the whole code is outdated and no longer applicable, as Java 7 is already minimum requirement for Hadoop 2.7.x. So such buggy code parts still lingering around is a big issue in open source projects like hadoop, especially in static initializers.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          This is the shell Java version probe in Hadoop 2.8.0:

            /**
             * query to see if system is Java 7 or later.
             * Now that Hadoop requires Java 7 or later, this always returns true.
             * @deprecated This call isn't needed any more: please remove uses of it.
             * @return true, always.
             */
            @Deprecated
            public static boolean isJava7OrAbove() {
              return true;
            }
          

          If this probe is the core problem, well, it can be cherry picked; along with where the references have ll been cut. HADOOP-10775 was the patch, though it's a bit big as it's trying to improve shell's failure handling. Marking as related to that, though it could be a dupe/part-of.

          Show
          stevel@apache.org Steve Loughran added a comment - This is the shell Java version probe in Hadoop 2.8.0: /** * query to see if system is Java 7 or later. * Now that Hadoop requires Java 7 or later, this always returns true . * @deprecated This call isn't needed any more: please remove uses of it. * @ return true , always. */ @Deprecated public static boolean isJava7OrAbove() { return true ; } If this probe is the core problem, well, it can be cherry picked; along with where the references have ll been cut. HADOOP-10775 was the patch, though it's a bit big as it's trying to improve shell's failure handling. Marking as related to that, though it could be a dupe/part-of.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          ps

          So such buggy code parts still lingering around is a big issue in open source projects like hadoop, especially in static initializers.

          it isn't lingering around; we cut it in 2015. Please upgrade

          Show
          stevel@apache.org Steve Loughran added a comment - ps So such buggy code parts still lingering around is a big issue in open source projects like hadoop, especially in static initializers. it isn't lingering around; we cut it in 2015. Please upgrade
          Hide
          thetaphi Uwe Schindler added a comment -

          it isn't lingering around; we cut it in 2015. Please upgrade

          Working on that, but we have a problem with the test-only MiniDFSCluster (see above). Checking on Maven Central, hadoop-common-2.7 is used at many places, so Solr is not the only lib that uses it.

          Show
          thetaphi Uwe Schindler added a comment - it isn't lingering around; we cut it in 2015. Please upgrade Working on that, but we have a problem with the test-only MiniDFSCluster (see above). Checking on Maven Central, hadoop-common-2.7 is used at many places, so Solr is not the only lib that uses it.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          I understand, but I still think you should have at least done a diff in your IDE of the 2.7 code vs. current code before making claims like " the whole code is outdated and no longer applicable,".

          I really don't think that's an appropriate way to comment on bug reports, especially without doing the research. Even if the problem still existed in trunk, a bit of politeness would make me amenable to review. Instead I feel the next time you have a genuine defect with branch-2/trunk then I'll be unmotivated to look at. Sorry

          Show
          stevel@apache.org Steve Loughran added a comment - I understand, but I still think you should have at least done a diff in your IDE of the 2.7 code vs. current code before making claims like " the whole code is outdated and no longer applicable,". I really don't think that's an appropriate way to comment on bug reports, especially without doing the research. Even if the problem still existed in trunk, a bit of politeness would make me amenable to review. Instead I feel the next time you have a genuine defect with branch-2/trunk then I'll be unmotivated to look at. Sorry
          Hide
          thetaphi Uwe Schindler added a comment -

          Hi,
          Sorry please don't take it personal. You might know that with regard to migration to Java 9 I have a very hard standpoint. Sorry for being harsh, but at some point it gets disappointing when a lot of third party libraries use sun.misc.Unsafe everywhere, forget to add AccessController.doPrivileged, or spawn processes and do other stuff like broken checks in static initializers affecting applications and other libraries that depend on it.

          the whole code is outdated and no longer applicable,

          It is already outdated in branch 2.7, because according to Akira Ajisaka the 2.7.x Hadoop release was done with a minimum of Java 7. So when updating your code to Java 7 back at that time, the check should have been replaced from the beginning. I know, sometimes you miss to fix such code when you update your pom.xml to have another minimum Java version! Nevertheless, such type of code is a no-go, and I am still walking around and arguing about such type of code, because it hinders the adoption of Java 9 - and especially the Apache Lucene people are really pushing all our users to move to Java 9 as soon as it is out (for security reasons in Elasticsearch/Solr and also for performance reasons with MappedByteBuffers). Here is my huge problem with this type of code:

          • Never ever write code in static analyzers that may break (in this case the hardcoded substring(0,2)).
          • Don't expect strings that come from the outside to conform to any syntax / string length you expect.

          In that regard (I also see the Shell class and its usage everywhere as a big security risk), you should really fix the code - although in an outdated version. This is a serious issue, especially as hadoop-common-2.7 is still used in many projects. The problem is that any code that touches the Shell class (indirectly just by classloading) breaks! The fix is a one liner (the patch here is much too large). Just replace the whole line by a simple "true" and we are done, unrisky and well enough for a bugfix release.

          Uwe

          P.S.: Most of the stuff in the Shell class can be done with java.nio.file APIs (including symlinks, or instead of calling "df -h" to get all mount points and their free space), there is no operating system shell needed anymore. This may already be partially fixed in later versions of Hadoop, I just say: I am not the first one complaining about that. I know from several other issues that this is seen very critical from 3rd parties. This would also make it easier for people to install or run tests on platforms like windows. I don't know, why a 3rd party library like hadoop-common should ever spawn child processes without asking security manager or give the user an option to do it without. That's what I am arguing about. – And if the same class then also fails in its static initializer, causing a cascade of class-loading related problems, because the person who wrote the code did not ever think about an IndexOutOfBoundsException, I keep getting sarcastic, sorry.

          P.P.S.: What I write on twitter is unrelated to this issue and is my personal opinion!

          Show
          thetaphi Uwe Schindler added a comment - Hi, Sorry please don't take it personal. You might know that with regard to migration to Java 9 I have a very hard standpoint. Sorry for being harsh, but at some point it gets disappointing when a lot of third party libraries use sun.misc.Unsafe everywhere, forget to add AccessController.doPrivileged, or spawn processes and do other stuff like broken checks in static initializers affecting applications and other libraries that depend on it. the whole code is outdated and no longer applicable, It is already outdated in branch 2.7, because according to Akira Ajisaka the 2.7.x Hadoop release was done with a minimum of Java 7. So when updating your code to Java 7 back at that time, the check should have been replaced from the beginning. I know, sometimes you miss to fix such code when you update your pom.xml to have another minimum Java version! Nevertheless, such type of code is a no-go, and I am still walking around and arguing about such type of code, because it hinders the adoption of Java 9 - and especially the Apache Lucene people are really pushing all our users to move to Java 9 as soon as it is out (for security reasons in Elasticsearch/Solr and also for performance reasons with MappedByteBuffers). Here is my huge problem with this type of code: Never ever write code in static analyzers that may break (in this case the hardcoded substring(0,2) ). Don't expect strings that come from the outside to conform to any syntax / string length you expect. In that regard (I also see the Shell class and its usage everywhere as a big security risk), you should really fix the code - although in an outdated version. This is a serious issue, especially as hadoop-common-2.7 is still used in many projects. The problem is that any code that touches the Shell class (indirectly just by classloading) breaks! The fix is a one liner (the patch here is much too large). Just replace the whole line by a simple "true" and we are done, unrisky and well enough for a bugfix release. Uwe P.S.: Most of the stuff in the Shell class can be done with java.nio.file APIs (including symlinks, or instead of calling "df -h" to get all mount points and their free space), there is no operating system shell needed anymore. This may already be partially fixed in later versions of Hadoop, I just say: I am not the first one complaining about that. I know from several other issues that this is seen very critical from 3rd parties. This would also make it easier for people to install or run tests on platforms like windows. I don't know, why a 3rd party library like hadoop-common should ever spawn child processes without asking security manager or give the user an option to do it without. That's what I am arguing about. – And if the same class then also fails in its static initializer, causing a cascade of class-loading related problems, because the person who wrote the code did not ever think about an IndexOutOfBoundsException, I keep getting sarcastic, sorry. P.P.S.: What I write on twitter is unrelated to this issue and is my personal opinion!
          Hide
          aw Allen Wittenauer added a comment -

          Is this worth fixing in 2.7.x given the general direction that 3.x (or 4.x?) will likely end up being the officially sanctioned "Java 9" compatible release? (See HADOOP-11123 for more.)

          Show
          aw Allen Wittenauer added a comment - Is this worth fixing in 2.7.x given the general direction that 3.x (or 4.x?) will likely end up being the officially sanctioned "Java 9" compatible release? (See HADOOP-11123 for more.)
          Hide
          ajisakaa Akira Ajisaka added a comment -

          Probably Apache Hadoop won't support Java 9 in Hadoop 2.7.x, but I'm thinking it's worth fixing this.

          • Fixing this issue is easy and the fix is compatible
          • Users want the fix
          • Now branch-2.7 is the only branch that is stable and maintained, so I'm thinking we cannot force the users to upgrade the dependency to 2.8.x or upper.
          Show
          ajisakaa Akira Ajisaka added a comment - Probably Apache Hadoop won't support Java 9 in Hadoop 2.7.x, but I'm thinking it's worth fixing this. Fixing this issue is easy and the fix is compatible Users want the fix Now branch-2.7 is the only branch that is stable and maintained, so I'm thinking we cannot force the users to upgrade the dependency to 2.8.x or upper.
          Hide
          thetaphi Uwe Schindler added a comment - - edited

          Hi,
          I would not backport anything. Just change the line to return true in 2.7.x. At least remove the substring(0,2) from it, This is useless and has no effect on the result of the compareTo call.
          The problem we have is:

          • Solr claims to be compatible to Java 9 (already in the 6.x branch). And in fact it is, unless you somehow hit the class accidentally by adding a HdfsDirectoryFactory or Kerberos Authentication to your config. It then breaks (it is enough to have it in the "example" config).
          • The fix is easy and we cannot upgrade to 2.8.x in the existing bugfix branch (Solr 6.6)
          • For master (coming Lucene/Solr 7.0), we are working on fixing this

          If we cannot fix this easily, we would fork the Hadoop 2.7 branch and make a solr-private release on Maven Central with just the one line changed, so we can fix this serious bug - and I don't like this to happen. It has in reality nothing to do with Java 9, it is just that some code in a static initializer applies an operation on a string that comes from the outside without correct bounds checks. This is a bug that may happen in any Java version out there, not only 9. Think of custom builds of OpenJDK.

          Show
          thetaphi Uwe Schindler added a comment - - edited Hi, I would not backport anything. Just change the line to return true in 2.7.x. At least remove the substring(0,2) from it, This is useless and has no effect on the result of the compareTo call. The problem we have is: Solr claims to be compatible to Java 9 (already in the 6.x branch). And in fact it is, unless you somehow hit the class accidentally by adding a HdfsDirectoryFactory or Kerberos Authentication to your config. It then breaks (it is enough to have it in the "example" config). The fix is easy and we cannot upgrade to 2.8.x in the existing bugfix branch (Solr 6.6) For master (coming Lucene/Solr 7.0), we are working on fixing this If we cannot fix this easily, we would fork the Hadoop 2.7 branch and make a solr-private release on Maven Central with just the one line changed, so we can fix this serious bug - and I don't like this to happen. It has in reality nothing to do with Java 9, it is just that some code in a static initializer applies an operation on a string that comes from the outside without correct bounds checks. This is a bug that may happen in any Java version out there, not only 9. Think of custom builds of OpenJDK.
          Hide
          shv Konstantin Shvachko added a comment - - edited

          Hi Uwe Schindler, the best way would be to submit a patch that you think fixes Solr issue on Java 9 and make sure that it doesn't break Hadoop on Java 7 and 8.
          Or verify that attached patch works for you.

          Show
          shv Konstantin Shvachko added a comment - - edited Hi Uwe Schindler , the best way would be to submit a patch that you think fixes Solr issue on Java 9 and make sure that it doesn't break Hadoop on Java 7 and 8. Or verify that attached patch works for you.
          Hide
          thetaphi Uwe Schindler added a comment -

          Hi,

          this is the simplest patch possible, just fixing this bug, not backporting anything! This would solve the issue and does not involve any changes in code.

          This patch only changes the constant IS_JAVA7_OR_ABOVE to true. As Hadoop 2.7 is compiled against Java 7, this will always be true.

          Show
          thetaphi Uwe Schindler added a comment - Hi, this is the simplest patch possible, just fixing this bug, not backporting anything! This would solve the issue and does not involve any changes in code. This patch only changes the constant IS_JAVA7_OR_ABOVE to true . As Hadoop 2.7 is compiled against Java 7, this will always be true.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          I want the patch which selectively cherry picks the minimum bits of the 2.8 code, particular that const and the IsJava7OrAbove() function call. We could get away with the other obsolete cruft.

          Why? I don't want 2.7.x to actually add new code which isn't a backport

          Show
          stevel@apache.org Steve Loughran added a comment - I want the patch which selectively cherry picks the minimum bits of the 2.8 code, particular that const and the IsJava7OrAbove() function call. We could get away with the other obsolete cruft. Why? I don't want 2.7.x to actually add new code which isn't a backport
          Hide
          thetaphi Uwe Schindler added a comment - - edited

          Hi Steve,

          I want the patch which selectively cherry picks the minimum bits of the 2.8 code, particular that const and the IsJava7OrAbove() function call. We could get away with the other obsolete cruft.

          I would go minimal here. Removeing a lot of obsolete cruft is to risky in 2.7 (that's my opinion). If the constant is static and final, the optimizer will remove the cruft anyways

          Nevertheless, I am fine with both ideas! My latest patch is just a small change without any actual code change. It just changes the constant value to hardcoded "true" instead of the failing property check that will always return "true" based on the Hadoop 2.7 minimum Java requirements. To me this is just a "bugfix" on the constant definition in Hadoop 2.7, no longer applicable to later versions.

          Why? I don't want 2.7.x to actually add new code which isn't a backport

          That's the reason why I submitted my patch. It does not change any code/logic, it just replaces a function call by a constant value, which is constant anyways.

          I agree that the whole issue only affects Hadoop 2.7 and before, so I am sorry for initially arguing that also Hadoop 2.8 or 3.0 has the same problem. I should have checked the code, but I was not aware of any changes. As I am not a Hadoop developer, I did not checkout any code, I just looked at the source code as returned by my IDE fom Maven Central. Sorry for getting aggressive, but I hope you agree that code like we have seen here should have never been in Hadoop!

          Finally I just repeat: This is issue is not dircetly related to Java 9, the attached commit just fixes "buggy code": It misses to add bounds checks when parsing a string that is not controlled by Hadoop - coming from the outside. This may be fixed in Hadoop 2.8, but 2.7 is still under support and the commit logs show that a lot of stuff is committed to the 2.7 branch.

          Show
          thetaphi Uwe Schindler added a comment - - edited Hi Steve, I want the patch which selectively cherry picks the minimum bits of the 2.8 code, particular that const and the IsJava7OrAbove() function call. We could get away with the other obsolete cruft. I would go minimal here. Removeing a lot of obsolete cruft is to risky in 2.7 (that's my opinion). If the constant is static and final, the optimizer will remove the cruft anyways Nevertheless, I am fine with both ideas! My latest patch is just a small change without any actual code change. It just changes the constant value to hardcoded "true" instead of the failing property check that will always return "true" based on the Hadoop 2.7 minimum Java requirements. To me this is just a "bugfix" on the constant definition in Hadoop 2.7, no longer applicable to later versions. Why? I don't want 2.7.x to actually add new code which isn't a backport That's the reason why I submitted my patch. It does not change any code/logic, it just replaces a function call by a constant value, which is constant anyways. I agree that the whole issue only affects Hadoop 2.7 and before, so I am sorry for initially arguing that also Hadoop 2.8 or 3.0 has the same problem. I should have checked the code, but I was not aware of any changes. As I am not a Hadoop developer, I did not checkout any code, I just looked at the source code as returned by my IDE fom Maven Central. Sorry for getting aggressive, but I hope you agree that code like we have seen here should have never been in Hadoop! Finally I just repeat: This is issue is not dircetly related to Java 9, the attached commit just fixes "buggy code": It misses to add bounds checks when parsing a string that is not controlled by Hadoop - coming from the outside. This may be fixed in Hadoop 2.8, but 2.7 is still under support and the commit logs show that a lot of stuff is committed to the 2.7 branch.
          Hide
          aw Allen Wittenauer added a comment -

          Cleaned up a bunch of stuff in the description, including lowering the priority since Apache Hadoop 2.7 on JDK9 isn't a configuration that the community has particularly targeted as viable.

          Show
          aw Allen Wittenauer added a comment - Cleaned up a bunch of stuff in the description, including lowering the priority since Apache Hadoop 2.7 on JDK9 isn't a configuration that the community has particularly targeted as viable.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Allen Wittenauer I know we don't support java 9 in hadoop itself, and will never do so in hadoop 2.7, but using the core libs for client-side operation is something more realistic as an initial deliverable.

          Show
          stevel@apache.org Steve Loughran added a comment - Allen Wittenauer I know we don't support java 9 in hadoop itself, and will never do so in hadoop 2.7, but using the core libs for client-side operation is something more realistic as an initial deliverable.
          Hide
          thetaphi Uwe Schindler added a comment - - edited

          Hi,
          Thanks Steve Loughran - this is the problem we had. We do not install Hadoop anywhere and we do not run Hadoop Clusters inside Apache Solr, we are just users using the client library (and parts of its tests, e.g. the MiniDFSCluster for testing - which works on Java 9). The used client library has a bug that breaks a component that uses it. So - as said before, I'd not backport anything complex, I'd just apply the bugfix simple patch that changes the static constant to "true" and all should be happy. There is no risk at all, as Hadoop 2.7 has a minimum requirement of Java 7, so the constant is "true".

          As users of the hadoop client library: We just don't want to break our "Java 9 compatible" Solr Core that uses the Hadoop libraries for some authentication stuff and allows to store indexes on HDFS. Another problem why we don't necessarily want to upgrade Hadoop in stable Solr (6.x), because how behaves a newer Hadoop client when talking to an older version. So we can't just raise version number.

          We don't tell our users that you can use Solr with Hadoop on Java 9, but Solr on its own should run with Java 9 and dependent libraries should not fail on classloading. So we have two possibilities:

          • Remove all of hadoop support from Solr Core (not a good idea)
          • Just fix the "Shell" class with a one-liner so it can actually just "load" successfully on Java 9 and not break other stuff (like StringUtils, Kerberos Auth,...). One possibility to do this is to fork hadoop-common in github and ship Solr with a patched version. But I don't like to do this!

          So I would really hope that you could apply the simple one-liner patch! Thanks

          Show
          thetaphi Uwe Schindler added a comment - - edited Hi, Thanks Steve Loughran - this is the problem we had. We do not install Hadoop anywhere and we do not run Hadoop Clusters inside Apache Solr, we are just users using the client library (and parts of its tests, e.g. the MiniDFSCluster for testing - which works on Java 9). The used client library has a bug that breaks a component that uses it. So - as said before, I'd not backport anything complex, I'd just apply the bugfix simple patch that changes the static constant to "true" and all should be happy. There is no risk at all, as Hadoop 2.7 has a minimum requirement of Java 7, so the constant is "true". As users of the hadoop client library: We just don't want to break our "Java 9 compatible" Solr Core that uses the Hadoop libraries for some authentication stuff and allows to store indexes on HDFS. Another problem why we don't necessarily want to upgrade Hadoop in stable Solr (6.x), because how behaves a newer Hadoop client when talking to an older version. So we can't just raise version number. We don't tell our users that you can use Solr with Hadoop on Java 9, but Solr on its own should run with Java 9 and dependent libraries should not fail on classloading. So we have two possibilities: Remove all of hadoop support from Solr Core (not a good idea) Just fix the "Shell" class with a one-liner so it can actually just "load" successfully on Java 9 and not break other stuff (like StringUtils, Kerberos Auth,...). One possibility to do this is to fork hadoop-common in github and ship Solr with a patched version. But I don't like to do this! So I would really hope that you could apply the simple one-liner patch! Thanks
          Hide
          thetaphi Uwe Schindler added a comment -

          Hi,
          as we are a bit under pressure at Solr, I have developed a quick hack around the problem. It is currently testing on our Linux build servers with Java 9 b175: https://github.com/apache/lucene-solr/compare/master...uschindler:jira/SOLR-10951-2

          Of course this is a really horrible hack, but it "forces" class initialization on early startup time of Solr and temporarily fakes the java.version system property.

          We are still hoping for a non-hacky solution.

          Show
          thetaphi Uwe Schindler added a comment - Hi, as we are a bit under pressure at Solr, I have developed a quick hack around the problem. It is currently testing on our Linux build servers with Java 9 b175: https://github.com/apache/lucene-solr/compare/master...uschindler:jira/SOLR-10951-2 Of course this is a really horrible hack, but it "forces" class initialization on early startup time of Solr and temporarily fakes the java.version system property. We are still hoping for a non-hacky solution.
          Hide
          aw Allen Wittenauer added a comment -

          using the core libs for client-side operation is something more realistic as an initial deliverable.

          Understood, but if you look at the list of things that we know are broken when running under JDK9 (even the client-side bits), they are never likely to get fully fixed in 2.7. It's very misleading to downstream to indicate that JDK9 will ever fully work properly with 2.7, and maybe even 2.8 or 2.9. Especially when considering the release rate for branch-2....

          Show
          aw Allen Wittenauer added a comment - using the core libs for client-side operation is something more realistic as an initial deliverable. Understood, but if you look at the list of things that we know are broken when running under JDK9 (even the client-side bits), they are never likely to get fully fixed in 2.7. It's very misleading to downstream to indicate that JDK9 will ever fully work properly with 2.7, and maybe even 2.8 or 2.9. Especially when considering the release rate for branch-2....
          Hide
          thetaphi Uwe Schindler added a comment -

          The workaround is this issue: SOLR-10966

          With this silly hack, Hadoop's client library 2.7.2 works without any issues for:

          • HDFS integration
          • Hadoop Authentication
          • Kerberos Auth (somehow uses also hadoop-common, maybe through 3rd party dependency hell)

          Here is the running test: https://jenkins.thetaphi.de/job/Lucene-Solr-Hadoop-Update/19/console

          Show
          thetaphi Uwe Schindler added a comment - The workaround is this issue: SOLR-10966 With this silly hack, Hadoop's client library 2.7.2 works without any issues for: HDFS integration Hadoop Authentication Kerberos Auth (somehow uses also hadoop-common, maybe through 3rd party dependency hell) Here is the running test: https://jenkins.thetaphi.de/job/Lucene-Solr-Hadoop-Update/19/console
          Hide
          shv Konstantin Shvachko added a comment - - edited

          Hey Uwe Schindler can we just replace substring().compareTo() with startsWith()? This will leave the semantics equivalent and should get rid of IndexOutOfBoundsException for Java 9.
          I'll be glad to commit this to 2.7.4.
          Nice hack btw.

          Show
          shv Konstantin Shvachko added a comment - - edited Hey Uwe Schindler can we just replace substring().compareTo() with startsWith() ? This will leave the semantics equivalent and should get rid of IndexOutOfBoundsException for Java 9. I'll be glad to commit this to 2.7.4. Nice hack btw.
          Hide
          thetaphi Uwe Schindler added a comment -

          Hey Uwe Schindler can we just replace substring().compareTo() with startsWith()? This will leave the semantics equivalent and should get rid of IndexOutOfBoundsException for Java 9.

          Sorry, no! The semantics are different! A simple startsWith would limit true to Java 7, Java 8 would result in false. The compareTo in the original code uses >= 0. T correct fix is much easier: Replace the whole line by true as I did in my patch. It can never get false, because the minimum Java version for compiling Hadoop 2.7 is Java 7, so it's true whatever happens (unless it breaks with Java 9, as it does without a fix).

          Show
          thetaphi Uwe Schindler added a comment - Hey Uwe Schindler can we just replace substring().compareTo() with startsWith()? This will leave the semantics equivalent and should get rid of IndexOutOfBoundsException for Java 9. Sorry, no! The semantics are different! A simple startsWith would limit true to Java 7, Java 8 would result in false . The compareTo in the original code uses >= 0 . T correct fix is much easier: Replace the whole line by true as I did in my patch. It can never get false, because the minimum Java version for compiling Hadoop 2.7 is Java 7, so it's true whatever happens (unless it breaks with Java 9, as it does without a fix).
          Hide
          shv Konstantin Shvachko added a comment -

          You are right it's lexicographic order.
          I tried to compile 2.7 branch with Java 1.6 and it failed via maven enforcer plugin.

          [INFO] --- maven-enforcer-plugin:1.3.1:enforce (clean) @ hadoop-main ---
          [WARNING] Rule 1: org.apache.maven.plugins.enforcer.RequireJavaVersion failed with message:
          Detected JDK Version: 1.6.0-65 is not in the allowed range [1.7,).
          

          So I think it is safe to return true here. Could you please add a JavaDoc comment for IS_JAVA7_OR_ABOVE that Java 1.7 or higher is enforced in pom.xml.

          Show
          shv Konstantin Shvachko added a comment - You are right it's lexicographic order. I tried to compile 2.7 branch with Java 1.6 and it failed via maven enforcer plugin. [INFO] --- maven-enforcer-plugin:1.3.1:enforce (clean) @ hadoop-main --- [WARNING] Rule 1: org.apache.maven.plugins.enforcer.RequireJavaVersion failed with message: Detected JDK Version: 1.6.0-65 is not in the allowed range [1.7,). So I think it is safe to return true here. Could you please add a JavaDoc comment for IS_JAVA7_OR_ABOVE that Java 1.7 or higher is enforced in pom.xml.
          Hide
          stevel@apache.org Steve Loughran added a comment -

          Konstantin: Uwe is right, the probe isn't needed. 2.7 is Java 7+. 2.8 contains the patch which removes the probe as part of a big cleanup of Shell itself. Cherry picking the smallest bits of that patch needed to fix the condition and remove some codepaths which can be guaranteed never to be executed would make this problem go away

          Show
          stevel@apache.org Steve Loughran added a comment - Konstantin: Uwe is right, the probe isn't needed. 2.7 is Java 7+. 2.8 contains the patch which removes the probe as part of a big cleanup of Shell itself. Cherry picking the smallest bits of that patch needed to fix the condition and remove some codepaths which can be guaranteed never to be executed would make this problem go away
          Hide
          shv Konstantin Shvachko added a comment -

          He Steve. I really don't like the idea of cherry picking parts of a patch. It makes it harder to backport when you really need that change as a whole.
          I like the simplicity of Uwe Schindler' change. We just need a JavaDoc comment to minimize confusion.

          Show
          shv Konstantin Shvachko added a comment - He Steve. I really don't like the idea of cherry picking parts of a patch. It makes it harder to backport when you really need that change as a whole. I like the simplicity of Uwe Schindler ' change. We just need a JavaDoc comment to minimize confusion.
          Hide
          shv Konstantin Shvachko added a comment -

          +1 on Uwe's patch. Added JavaDoc comment. Will commit if there are no objections.

          Show
          shv Konstantin Shvachko added a comment - +1 on Uwe's patch. Added JavaDoc comment. Will commit if there are no objections.
          Hide
          thetaphi Uwe Schindler added a comment -

          Thanks! +1
          Sorry for not providing a new patch with an additional comment, but I already removed my checkout of hadoop from my local machine.

          Show
          thetaphi Uwe Schindler added a comment - Thanks! +1 Sorry for not providing a new patch with an additional comment, but I already removed my checkout of hadoop from my local machine.
          Hide
          shv Konstantin Shvachko added a comment -

          I just committed this to branch-2.7. Thank you Uwe Schindler.

          Show
          shv Konstantin Shvachko added a comment - I just committed this to branch-2.7. Thank you Uwe Schindler .
          Hide
          thetaphi Uwe Schindler added a comment -

          FYI, I updated Solr to use Hadoop 2.7.4. Thanks!

          Show
          thetaphi Uwe Schindler added a comment - FYI, I updated Solr to use Hadoop 2.7.4. Thanks!

            People

            • Assignee:
              ajisakaa Akira Ajisaka
              Reporter:
              thetaphi Uwe Schindler
            • Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development