Solr
  1. Solr
  2. SOLR-7024

bin/solr: Improve java detection and error messages

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 5.0
    • Fix Version/s: 5.0, 6.0
    • Component/s: scripts and tools
    • Labels:
      None
    • Environment:

      Linux bigindy5 3.10.0-123.9.2.el7.x86_64 #1 SMP Tue Oct 28 18:05:26 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux

      Description

      Java detection needs a bit of an overhaul. One example: When running the shell script, if JAVA_HOME is set, but does not point to a valid java home, Solr will not start, but the error message is unhelpful, especially to users who actually DO have the right java version installed.

      1. SOLR-7024.patch
        2 kB
        Shawn Heisey
      2. SOLR-7024.patch
        2 kB
        Shawn Heisey
      3. SOLR-7024.patch
        2 kB
        Shawn Heisey
      4. SOLR-7024.patch
        2 kB
        Shawn Heisey
      5. SOLR-7024.patch
        2 kB
        Shawn Heisey
      6. SOLR-7024.patch
        2 kB
        Shawn Heisey
      7. SOLR-7024.patch
        2 kB
        Shawn Heisey
      8. SOLR-7024.patch
        2 kB
        Shawn Heisey
      9. SOLR-7024.patch
        2 kB
        Shawn Heisey
      10. SOLR-7024.patch
        1.0 kB
        Shawn Heisey

        Issue Links

          Activity

          Hide
          Anshum Gupta added a comment -

          I'm not sure if this is expected/wanted. What happens when JAVA_HOME is not set at all? I think that should be the failover too but at the same time, it should at least echo a WARNING in such a case before the auto-failover happens.

          Show
          Anshum Gupta added a comment - I'm not sure if this is expected/wanted. What happens when JAVA_HOME is not set at all? I think that should be the failover too but at the same time, it should at least echo a WARNING in such a case before the auto-failover happens.
          Hide
          Timothy Potter added a comment -

          Don't see how this is a blocker?

          $ export JAVA_HOME=not_a_valid_path
          $ bin/solr start
          Java is required to run Solr! Please install Java 7 or 8 before running this script.
          
          Show
          Timothy Potter added a comment - Don't see how this is a blocker? $ export JAVA_HOME=not_a_valid_path $ bin/solr start Java is required to run Solr! Please install Java 7 or 8 before running this script.
          Hide
          Alexandre Rafalovitch added a comment -

          I think it should crash/ERROR in that case. Otherwise, it would be impossible to troubleshoot. JAVA_HOME is supposed to be an override of sorts.

          Show
          Alexandre Rafalovitch added a comment - I think it should crash/ERROR in that case. Otherwise, it would be impossible to troubleshoot. JAVA_HOME is supposed to be an override of sorts.
          Hide
          Shawn Heisey added a comment -

          Patch that fixes the problem, and also adds troubleshooting info to the "java not found" error message.

          Show
          Shawn Heisey added a comment - Patch that fixes the problem, and also adds troubleshooting info to the "java not found" error message.
          Hide
          Shawn Heisey added a comment -

          Patch with entry in CHANGES.txt

          Show
          Shawn Heisey added a comment - Patch with entry in CHANGES.txt
          Hide
          Shawn Heisey added a comment - - edited

          Timothy Potter If a good java version is available on the PATH, it does not get found, because JAVA is never set when JAVA_HOME refers to a location that doesn't contain java.

          Blocker might be overkill.

          Show
          Shawn Heisey added a comment - - edited Timothy Potter If a good java version is available on the PATH, it does not get found, because JAVA is never set when JAVA_HOME refers to a location that doesn't contain java. Blocker might be overkill.
          Hide
          Anshum Gupta added a comment - - edited

          +1 on what Alexandre Rafalovitch said. Considering JAVA_HOME is supposed to be an override, a wrong override must result in an error..

          Not having JAVA_HOME defined and then finding JAVA in the path is totally fine in my opinion though.

          Show
          Anshum Gupta added a comment - - edited +1 on what Alexandre Rafalovitch said. Considering JAVA_HOME is supposed to be an override, a wrong override must result in an error.. Not having JAVA_HOME defined and then finding JAVA in the path is totally fine in my opinion though.
          Hide
          Shawn Heisey added a comment -

          I guess we need to decide what we want to happen when we have an invalid JAVA_HOME.

          Show
          Shawn Heisey added a comment - I guess we need to decide what we want to happen when we have an invalid JAVA_HOME.
          Hide
          Shawn Heisey added a comment -

          New patch. If JAVA_HOME is no good, lets the user know the script is going to try to find java on the path.

          Show
          Shawn Heisey added a comment - New patch. If JAVA_HOME is no good, lets the user know the script is going to try to find java on the path.
          Hide
          Shawn Heisey added a comment -

          Alternately, the message in the new patch could just indicate JAVA_HOME is no good and exit.

          Show
          Shawn Heisey added a comment - Alternately, the message in the new patch could just indicate JAVA_HOME is no good and exit.
          Hide
          Shawn Heisey added a comment -

          Typo in patch.
          "an location" -> "a location"

          Show
          Shawn Heisey added a comment - Typo in patch. "an location" -> "a location"
          Hide
          Anshum Gupta added a comment - - edited

          I'd suggest exiting out after mentioning (which happens already I think).
          Overriding an override might not be such a good idea.

          We could change the message to something more descriptive though so instead of "Please install Java 7 or 8 before running this script.", we could provide information about the JAVA_HOME that was tried and that it was an invalid path.

          Show
          Anshum Gupta added a comment - - edited I'd suggest exiting out after mentioning (which happens already I think). Overriding an override might not be such a good idea. We could change the message to something more descriptive though so instead of "Please install Java 7 or 8 before running this script.", we could provide information about the JAVA_HOME that was tried and that it was an invalid path.
          Hide
          Shawn Heisey added a comment -

          New patch.

          Cleaned up the new echo commands and improved the existing error message. Script aborts if java not located in JAVA_HOME location.

          Show
          Shawn Heisey added a comment - New patch. Cleaned up the new echo commands and improved the existing error message. Script aborts if java not located in JAVA_HOME location.
          Hide
          Shawn Heisey added a comment -

          CHANGES.txt update.

          Show
          Shawn Heisey added a comment - CHANGES.txt update.
          Hide
          Shawn Heisey added a comment -

          forgot to include my name on the patch, but if this form is acceptable, it will be present in what gets committed.

          Show
          Shawn Heisey added a comment - forgot to include my name on the patch, but if this form is acceptable, it will be present in what gets committed.
          Hide
          Shawn Heisey added a comment -

          Updated patch. I had changed the default executable to javxa so I could verify functionality, that edit was left in.

          Show
          Shawn Heisey added a comment - Updated patch. I had changed the default executable to javxa so I could verify functionality, that edit was left in.
          Hide
          Shawn Heisey added a comment -

          Another iteration of the patch. This only has a minor update to CHANGES.txt.

          Show
          Shawn Heisey added a comment - Another iteration of the patch. This only has a minor update to CHANGES.txt.
          Hide
          Anshum Gupta added a comment -

          Do we really need to init the variable as BADJAVA? Just checking against if [ -z "$JAVA" ] should work too, right?
          Also, we should have this fix for windows too so that the behavior stays consistent.

          Show
          Anshum Gupta added a comment - Do we really need to init the variable as BADJAVA? Just checking against if [ -z "$JAVA" ] should work too, right? Also, we should have this fix for windows too so that the behavior stays consistent.
          Hide
          Shawn Heisey added a comment -

          I promise that I will eventually have the patch where I want it.

          Tweaked the error message a little.

          Show
          Shawn Heisey added a comment - I promise that I will eventually have the patch where I want it. Tweaked the error message a little.
          Hide
          Shawn Heisey added a comment -

          Good idea on using -z. New patch that does exactly that.

          I looked at the windows batch file. The logic there looks completely different and I'm not sure at first glance how to make a similar change.

          Show
          Shawn Heisey added a comment - Good idea on using -z. New patch that does exactly that. I looked at the windows batch file. The logic there looks completely different and I'm not sure at first glance how to make a similar change.
          Hide
          Anshum Gupta added a comment -

          Please install/fix Java 7 or 8 before running this script.

          You might want to rename this to:
          Please install Java 7 or 8 and fix JAVA_HOME before running this script?

          Also, windows? did you mean you'd have a patch for that too?

          Show
          Anshum Gupta added a comment - Please install/fix Java 7 or 8 before running this script. You might want to rename this to: Please install Java 7 or 8 and fix JAVA_HOME before running this script? Also, windows? did you mean you'd have a patch for that too?
          Hide
          Shawn Heisey added a comment -

          Moving out of Blocker because I don't have any solution for Windows yet.

          Show
          Shawn Heisey added a comment - Moving out of Blocker because I don't have any solution for Windows yet.
          Hide
          Shawn Heisey added a comment -

          I will create a new issue for the batch file. I'd like to get the shell script improvements into 5.0, a similar improvement for Windows might need to wait until 5.0.1 or 5.1.

          Show
          Shawn Heisey added a comment - I will create a new issue for the batch file. I'd like to get the shell script improvements into 5.0, a similar improvement for Windows might need to wait until 5.0.1 or 5.1.
          Hide
          ASF subversion and git services added a comment -

          Commit 1654434 from Shawn Heisey in branch 'dev/trunk'
          [ https://svn.apache.org/r1654434 ]

          SOLR-7024: bin/solr: Improve java detection and error messages

          Show
          ASF subversion and git services added a comment - Commit 1654434 from Shawn Heisey in branch 'dev/trunk' [ https://svn.apache.org/r1654434 ] SOLR-7024 : bin/solr: Improve java detection and error messages
          Hide
          ASF subversion and git services added a comment -

          Commit 1654436 from Shawn Heisey in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1654436 ]

          SOLR-7024: bin/solr: Improve java detection and error messages (merge trunk r1654434)

          Show
          ASF subversion and git services added a comment - Commit 1654436 from Shawn Heisey in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1654436 ] SOLR-7024 : bin/solr: Improve java detection and error messages (merge trunk r1654434)
          Hide
          ASF subversion and git services added a comment -

          Commit 1654438 from Shawn Heisey in branch 'dev/branches/lucene_solr_5_0'
          [ https://svn.apache.org/r1654438 ]

          SOLR-7024: bin/solr: Improve java detection and error messages (merge trunk r1654434)

          Show
          ASF subversion and git services added a comment - Commit 1654438 from Shawn Heisey in branch 'dev/branches/lucene_solr_5_0' [ https://svn.apache.org/r1654438 ] SOLR-7024 : bin/solr: Improve java detection and error messages (merge trunk r1654434)
          Hide
          Anshum Gupta added a comment -

          Bulk close after 5.0 release.

          Show
          Anshum Gupta added a comment - Bulk close after 5.0 release.
          Hide
          David Smiley added a comment -

          The bin/solr script on 5x will report an error to the user if Java isn't installed, but it will claim Java 8:

          echo >&2 "A working Java 8 is required to run Solr!"
          
          Show
          David Smiley added a comment - The bin/solr script on 5x will report an error to the user if Java isn't installed, but it will claim Java 8: echo >&2 "A working Java 8 is required to run Solr!"
          Hide
          ASF subversion and git services added a comment -

          Commit 1677094 from Shawn Heisey in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1677094 ]

          SOLR-7024: Correct Java version in error message to 7 or later on 5x.

          Show
          ASF subversion and git services added a comment - Commit 1677094 from Shawn Heisey in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1677094 ] SOLR-7024 : Correct Java version in error message to 7 or later on 5x.
          Hide
          Shawn Heisey added a comment -

          oops! I committed a fix.

          Show
          Shawn Heisey added a comment - oops! I committed a fix.

            People

            • Assignee:
              Shawn Heisey
              Reporter:
              Shawn Heisey
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development