Solr
  1. Solr
  2. SOLR-4791

solr.xml sharedLib does not work in 4.3.0

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.3
    • Fix Version/s: 4.4, 6.0
    • Component/s: multicore
    • Labels:
      None

      Description

      The sharedLib attribute on <solr> tag in solr.xml stopped working in 4.3.

      Using old-style solr.xml with sharedLib defined on solr tag. Solr does not load any of them. Simply swapping out solr.war with the 4.2.1 one, brings sharedLib loading back.

      1. closeLoader.patch
        3 kB
        Robert Muir
      2. SOLR-4791.patch
        11 kB
        Erick Erickson
      3. SOLR-4791.patch
        12 kB
        Ryan Ernst
      4. SOLR-4791.patch
        0.6 kB
        Ryan Ernst
      5. SOLR-4791-test.patch
        4 kB
        Erick Erickson

        Activity

        Hide
        Jan Høydahl added a comment -

        This is the log output

        40   [localhost-startStop-1] INFO  org.apache.solr.core.CoreContainer  – looking for solr config file: /Users/janhoy/git/openesp/build/openesp/bin/../conf/solr/solr.xml
        44   [localhost-startStop-1] INFO  org.apache.solr.core.CoreContainer  – New CoreContainer 218790518
        47   [localhost-startStop-1] INFO  org.apache.solr.core.CoreContainer  – Loading CoreContainer using Solr Home: '/Users/janhoy/git/openesp/build/openesp/bin/../conf/solr/'
        48   [localhost-startStop-1] INFO  org.apache.solr.core.SolrResourceLoader  – new SolrResourceLoader for directory: '/Users/janhoy/git/openesp/build/openesp/bin/../conf/solr/'
        373  [localhost-startStop-1] INFO  org.apache.solr.core.CoreContainer  – loading shared library: /Users/janhoy/git/openesp/build/openesp/bin/../lib/solr
        374  [localhost-startStop-1] INFO  org.apache.solr.core.SolrResourceLoader  – Adding 'file:/Users/janhoy/git/openesp/build/openesp/lib/solr/' to classloader
        

        ...it believes that the folder is a file... while in 4.2.1 it looks like this

        INFO: looking for solr.xml: /Users/janhoy/git/openesp/build/openesp/bin/../conf/solr/solr.xml
        mai 07, 2013 3:18:46 AM org.apache.solr.core.CoreContainer <init>
        INFO: New CoreContainer 39755063
        mai 07, 2013 3:18:46 AM org.apache.solr.core.CoreContainer load
        INFO: Loading CoreContainer using Solr Home: '/Users/janhoy/git/openesp/build/openesp/bin/../conf/solr/'
        (...)
        mai 07, 2013 3:18:46 AM org.apache.solr.core.CoreContainer load
        INFO: loading shared library: /Users/janhoy/git/openesp/build/openesp/bin/../lib/solr
        mai 07, 2013 3:18:46 AM org.apache.solr.core.SolrResourceLoader replaceClassLoader
        INFO: Adding 'file:/Users/janhoy/git/openesp/build/openesp/lib/solr/activation-1.1.jar' to classloader
        mai 07, 2013 3:18:46 AM org.apache.solr.core.SolrResourceLoader replaceClassLoader
        INFO: Adding 'file:/Users/janhoy/git/openesp/build/openesp/lib/solr/apache-mime4j-core-0.7.2.jar' to classloader
        
        Show
        Jan Høydahl added a comment - This is the log output 40 [localhost-startStop-1] INFO org.apache.solr.core.CoreContainer – looking for solr config file: /Users/janhoy/git/openesp/build/openesp/bin/../conf/solr/solr.xml 44 [localhost-startStop-1] INFO org.apache.solr.core.CoreContainer – New CoreContainer 218790518 47 [localhost-startStop-1] INFO org.apache.solr.core.CoreContainer – Loading CoreContainer using Solr Home: '/Users/janhoy/git/openesp/build/openesp/bin/../conf/solr/' 48 [localhost-startStop-1] INFO org.apache.solr.core.SolrResourceLoader – new SolrResourceLoader for directory: '/Users/janhoy/git/openesp/build/openesp/bin/../conf/solr/' 373 [localhost-startStop-1] INFO org.apache.solr.core.CoreContainer – loading shared library: /Users/janhoy/git/openesp/build/openesp/bin/../lib/solr 374 [localhost-startStop-1] INFO org.apache.solr.core.SolrResourceLoader – Adding 'file:/Users/janhoy/git/openesp/build/openesp/lib/solr/' to classloader ...it believes that the folder is a file... while in 4.2.1 it looks like this INFO: looking for solr.xml: /Users/janhoy/git/openesp/build/openesp/bin/../conf/solr/solr.xml mai 07, 2013 3:18:46 AM org.apache.solr.core.CoreContainer <init> INFO: New CoreContainer 39755063 mai 07, 2013 3:18:46 AM org.apache.solr.core.CoreContainer load INFO: Loading CoreContainer using Solr Home: '/Users/janhoy/git/openesp/build/openesp/bin/../conf/solr/' (...) mai 07, 2013 3:18:46 AM org.apache.solr.core.CoreContainer load INFO: loading shared library: /Users/janhoy/git/openesp/build/openesp/bin/../lib/solr mai 07, 2013 3:18:46 AM org.apache.solr.core.SolrResourceLoader replaceClassLoader INFO: Adding 'file:/Users/janhoy/git/openesp/build/openesp/lib/solr/activation-1.1.jar' to classloader mai 07, 2013 3:18:46 AM org.apache.solr.core.SolrResourceLoader replaceClassLoader INFO: Adding 'file:/Users/janhoy/git/openesp/build/openesp/lib/solr/apache-mime4j-core-0.7.2.jar' to classloader
        Hide
        Shawn Heisey added a comment -

        I upgraded my dev server to 4.3. If the sharedLib is set to "lib" (relative to solr.solr.home) then it does seem to work, although it's my understanding that even without a sharedLib setting, "lib" is checked, so that's a useless bit of config.

        Show
        Shawn Heisey added a comment - I upgraded my dev server to 4.3. If the sharedLib is set to "lib" (relative to solr.solr.home) then it does seem to work, although it's my understanding that even without a sharedLib setting, "lib" is checked, so that's a useless bit of config.
        Hide
        Jan Høydahl added a comment -

        Sure, ./lib is hardcoded, try another path

        Show
        Jan Høydahl added a comment - Sure, ./lib is hardcoded, try another path
        Hide
        Ryan Ernst added a comment -

        I believe the attached patch should fix the problem.

        I haven't come up with a test yet...would need to add a test solr.xml that specifies sharedLib (and uses a different name than "lib").

        On a side note, it is annoying that the 1 argument addToClassLoader expects a single file, while the 3 argument one expects a directory...

        Show
        Ryan Ernst added a comment - I believe the attached patch should fix the problem. I haven't come up with a test yet...would need to add a test solr.xml that specifies sharedLib (and uses a different name than "lib"). On a side note, it is annoying that the 1 argument addToClassLoader expects a single file, while the 3 argument one expects a directory...
        Hide
        Mark Miller added a comment -

        I think someone did recently write such a new test, right Erick? I'm pretty sure I saw a JIRA issue. Though it likely didn't specify an alternate lib location I would assume since it did not fail.

        Show
        Mark Miller added a comment - I think someone did recently write such a new test, right Erick? I'm pretty sure I saw a JIRA issue. Though it likely didn't specify an alternate lib location I would assume since it did not fail.
        Hide
        Ryan Ernst added a comment -

        Ok, I went down a rabbit hole and fixed another bug (if quiet was false, no files would ever be loaded in the 3 arg version of addToClassPath). I also removed the single arg version of addToClassPath, since it was only used in one place, and the documentation was wrong anyway (claimed it worked with a directory path, but only worked for a single file).

        I added two tests, one which tests adding paths to SolrResourceLoader, and another to ensure CoreContainer loads sharedLib correctly (and works with the default "lib").

        All of this is in the new patch.

        Show
        Ryan Ernst added a comment - Ok, I went down a rabbit hole and fixed another bug (if quiet was false, no files would ever be loaded in the 3 arg version of addToClassPath). I also removed the single arg version of addToClassPath, since it was only used in one place, and the documentation was wrong anyway (claimed it worked with a directory path, but only worked for a single file). I added two tests, one which tests adding paths to SolrResourceLoader, and another to ensure CoreContainer loads sharedLib correctly (and works with the default "lib"). All of this is in the new patch.
        Hide
        Erick Erickson added a comment -

        Ryan:

        Looks great! I'll check it in today trunk and 4x.....

        I took out a couple of unused imports I noticed that had probably been hanging around since forever but otherwise I didn't change anything....

        Show
        Erick Erickson added a comment - Ryan: Looks great! I'll check it in today trunk and 4x..... I took out a couple of unused imports I noticed that had probably been hanging around since forever but otherwise I didn't change anything....
        Hide
        Jan Høydahl added a comment -

        Tested the patch in my environment, works well (with old-style solr.xml, not tested with new style).

        Show
        Jan Høydahl added a comment - Tested the patch in my environment, works well (with old-style solr.xml, not tested with new style).
        Hide
        Jan Høydahl added a comment -

        Tested with new style solr.xml, and it works as well.

        Show
        Jan Høydahl added a comment - Tested with new style solr.xml, and it works as well.
        Hide
        Erick Erickson added a comment -

        trunk: 1480228
        4x: 1480252

        Thanks Ryan for the patch and Jan for testing!

        Ryan: My apologies, I forgot to add this to CHANGES.txt. I have another patch to commit sometime in the next couple of days and I'll add this (with attribution!) in then.

        Show
        Erick Erickson added a comment - trunk: 1480228 4x: 1480252 Thanks Ryan for the patch and Jan for testing! Ryan: My apologies, I forgot to add this to CHANGES.txt. I have another patch to commit sometime in the next couple of days and I'll add this (with attribution!) in then.
        Hide
        Erick Erickson added a comment -

        Apparently causing a test failure...

        Show
        Erick Erickson added a comment - Apparently causing a test failure...
        Hide
        Erick Erickson added a comment -

        Could someone with a windows machine try this patch out? Other then having to indent, all it does is try to delete the directory created in the new test. The only changed code is the adding try and the finally block. Passes on my Mac....

        Show
        Erick Erickson added a comment - Could someone with a windows machine try this patch out? Other then having to indent, all it does is try to delete the directory created in the new test. The only changed code is the adding try and the finally block. Passes on my Mac....
        Hide
        Robert Muir added a comment -

        Probably need to close() the classloader I think? ill test on a windows box if this does the trick

        Show
        Robert Muir added a comment - Probably need to close() the classloader I think? ill test on a windows box if this does the trick
        Hide
        Robert Muir added a comment -

        We can also only do this in trunk. there is no close() method in java6. So I think in branch_4x, the test should have an assumeFalse(Constants.WINDOWS)

        Show
        Robert Muir added a comment - We can also only do this in trunk. there is no close() method in java6. So I think in branch_4x, the test should have an assumeFalse(Constants.WINDOWS)
        Hide
        Robert Muir added a comment -

        totally untested patch. Ill try it on my windows machien in a few.

        I know that Uwe knows a trick for java6 (at least a best-effort we can do, which we should). But i still think we should have an assume() for windows there since it may not work on all jvms.

        Show
        Robert Muir added a comment - totally untested patch. Ill try it on my windows machien in a few. I know that Uwe knows a trick for java6 (at least a best-effort we can do, which we should). But i still think we should have an assume() for windows there since it may not work on all jvms.
        Hide
        Robert Muir added a comment -

        This works on my windows box with trunk (where it fails before). Ill take care of this...

        Show
        Robert Muir added a comment - This works on my windows box with trunk (where it fails before). Ill take care of this...
        Hide
        Erick Erickson added a comment -

        Thanks, man. I was taking a wild shot in the dark, obviously missing....

        Show
        Erick Erickson added a comment - Thanks, man. I was taking a wild shot in the dark, obviously missing....
        Hide
        Robert Muir added a comment -

        I committed the close()'ing (and a best-effort hack+assume). So I think we are now ok.

        Show
        Robert Muir added a comment - I committed the close()'ing (and a best-effort hack+assume). So I think we are now ok.
        Hide
        Uwe Schindler added a comment - - edited

        Thanks for referring to my Schindler-hack in the comment! I did this the same way in forbidden-apis, so the hack comes from there: https://code.google.com/p/forbidden-apis/source/browse/trunk/src/main/java/de/thetaphi/forbiddenapis/AbstractCheckMojo.java?r=153#229

        So I told this Robert privately (was on phone) how to handle URLClassLoader without reflecting methods. The addition of the Closeable interface to URLClassLoader is one of the "important" Java 7 bug fixes, which is also listed in Java 7's release notes (which was added to make e.g. Tomcat be able to remove the JAR files of a webapp when redeploying them on a windows container...).

        Show
        Uwe Schindler added a comment - - edited Thanks for referring to my Schindler-hack in the comment! I did this the same way in forbidden-apis, so the hack comes from there: https://code.google.com/p/forbidden-apis/source/browse/trunk/src/main/java/de/thetaphi/forbiddenapis/AbstractCheckMojo.java?r=153#229 So I told this Robert privately (was on phone) how to handle URLClassLoader without reflecting methods. The addition of the Closeable interface to URLClassLoader is one of the "important" Java 7 bug fixes, which is also listed in Java 7's release notes (which was added to make e.g. Tomcat be able to remove the JAR files of a webapp when redeploying them on a windows container...).
        Hide
        Shalin Shekhar Mangar added a comment -

        Is there anything left here? Can I backport the commits to 4.3.1?

        Show
        Shalin Shekhar Mangar added a comment - Is there anything left here? Can I backport the commits to 4.3.1?
        Hide
        Erick Erickson added a comment -

        Go for it. Be sure to apply both the SOLR-4791.patch and closeLoader.patch. They were originally applied in that order, but I doubt it matters.

        Show
        Erick Erickson added a comment - Go for it. Be sure to apply both the SOLR-4791 .patch and closeLoader.patch. They were originally applied in that order, but I doubt it matters.
        Hide
        Erick Erickson added a comment -

        Haven't seen any test failures since Robert added the patch so I'm marking this resolved.

        Show
        Erick Erickson added a comment - Haven't seen any test failures since Robert added the patch so I'm marking this resolved.
        Hide
        Robert Muir added a comment -

        shouldn't this have a changes entry?

        Show
        Robert Muir added a comment - shouldn't this have a changes entry?
        Hide
        Commit Tag Bot added a comment -

        [trunk commit] erick
        http://svn.apache.org/viewvc?view=revision&revision=1481038

        added to CHANGES.txt for SOLR-4791

        Show
        Commit Tag Bot added a comment - [trunk commit] erick http://svn.apache.org/viewvc?view=revision&revision=1481038 added to CHANGES.txt for SOLR-4791
        Hide
        Commit Tag Bot added a comment -

        [lucene_solr_4_3 commit] erick
        http://svn.apache.org/viewvc?view=revision&revision=1481043

        added to CHANGES.txt for SOLR-4791

        Show
        Commit Tag Bot added a comment - [lucene_solr_4_3 commit] erick http://svn.apache.org/viewvc?view=revision&revision=1481043 added to CHANGES.txt for SOLR-4791
        Hide
        Jan Høydahl added a comment -

        Testing the new 4.3.1 - still not getting sharedLib to work. Seems this issue was never backported to 4.3.1 even if the CHANGES entry was???

        Show
        Jan Høydahl added a comment - Testing the new 4.3.1 - still not getting sharedLib to work. Seems this issue was never backported to 4.3.1 even if the CHANGES entry was???
        Hide
        Erick Erickson added a comment -

        I certainly never backported it, and there's no commit bag bot entry that'd make me believe it was.....

        Erick

        Show
        Erick Erickson added a comment - I certainly never backported it, and there's no commit bag bot entry that'd make me believe it was..... Erick
        Hide
        Jan Høydahl added a comment -

        Ok, I see what happened, the CHANGES commit 1481043 went to lucene_solr_4_3, but it should have been on branch_4x. You fix?

        Show
        Jan Høydahl added a comment - Ok, I see what happened, the CHANGES commit 1481043 went to lucene_solr_4_3, but it should have been on branch_4x. You fix?
        Hide
        Shalin Shekhar Mangar added a comment -

        Ah, so what should we do now? I saw the commit bot entry on lucene_solr_4_3 and assumed that erick had backported this issue. The release vote has passed and artifacts are making their way into the mirrors.

        Show
        Shalin Shekhar Mangar added a comment - Ah, so what should we do now? I saw the commit bot entry on lucene_solr_4_3 and assumed that erick had backported this issue. The release vote has passed and artifacts are making their way into the mirrors.
        Hide
        Erick Erickson added a comment -

        I don't think we should re-spin the release at this point, or release a 4.3.2, especially when we're already talking about 4.4 relatively soon.

        Show
        Erick Erickson added a comment - I don't think we should re-spin the release at this point, or release a 4.3.2, especially when we're already talking about 4.4 relatively soon.
        Hide
        Erick Erickson added a comment -

        right, it missed 4.3.1 but will be in 4.4

        Erick

        Show
        Erick Erickson added a comment - right, it missed 4.3.1 but will be in 4.4 Erick
        Hide
        Steve Rowe added a comment -

        Bulk close resolved 4.4 issues

        Show
        Steve Rowe added a comment - Bulk close resolved 4.4 issues

          People

          • Assignee:
            Erick Erickson
            Reporter:
            Jan Høydahl
          • Votes:
            0 Vote for this issue
            Watchers:
            9 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development