Solr
  1. Solr
  2. SOLR-4817

Solr should not fall back to the back compat built in solr.xml in SolrCloud mode.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.5, Trunk
    • Component/s: SolrCloud
    • Labels:
      None

      Description

      A hard error is much more useful, and this built in solr.xml is not very good for solrcloud - with the old style solr.xml with cores in it, you won't have persistence and with the new style, it's not really ideal either.

      I think it makes it easier to debug solr.home to fail on this instead - but just in solrcloud mode for now due to back compat. We might want to pull the whole internal solr.xml for 5.0.

      1. SOLR-4817.patch
        24 kB
        Erick Erickson
      2. SOLR-4817.patch
        29 kB
        Erick Erickson
      3. SOLR-4817.patch
        34 kB
        Erick Erickson
      4. SOLR-4817.patch
        33 kB
        Erick Erickson
      5. SOLR-4817.patch
        34 kB
        Erick Erickson

        Activity

        Hide
        Adrien Grand added a comment -

        4.5 release -> bulk close

        Show
        Adrien Grand added a comment - 4.5 release -> bulk close
        Hide
        Erick Erickson added a comment -

        bq: I think it's all a bit of a mess right now

        Yeah, it certainly is but I haven't had the energy to try to straighten it out either. Maybe we can share some of the work....

        Show
        Erick Erickson added a comment - bq: I think it's all a bit of a mess right now Yeah, it certainly is but I haven't had the energy to try to straighten it out either. Maybe we can share some of the work....
        Hide
        Mark Miller added a comment -

        I think it's all a bit of a mess right now (the test configs situation) - we should clean this up more. I intend to take a crack at it at some point. It's still too haphazard what is done in what tests and too difficult to understand and follow when writing new tests or debugging old ones.

        Show
        Mark Miller added a comment - I think it's all a bit of a mess right now (the test configs situation) - we should clean this up more. I intend to take a crack at it at some point. It's still too haphazard what is done in what tests and too difficult to understand and follow when writing new tests or debugging old ones.
        Hide
        Shalin Shekhar Mangar added a comment -

        Just fyi, the copyMinConf, copyMinFullSetup and copySolrHomeToTemp methods throw the following exception with Solrj tests:

        junit4] ERROR 0.69s | MultiCoreExampleJettyTest.testDeleteInstanceDir <<<
        [junit4] > Throwable #1: java.lang.RuntimeException: Cannot find resource: /Users/shalinmangar/work/oss/solr-trunk/solr/build/solr-solrj/test/J0/solr/collection1
        [junit4] > at __randomizedtesting.SeedInfo.seed([2AFBC83FDA207BB2:4160F4A68E96AEF0]:0)
        [junit4] > at org.apache.solr.SolrTestCaseJ4.getFile(SolrTestCaseJ4.java:1571)
        [junit4] > at org.apache.solr.SolrTestCaseJ4.TEST_HOME(SolrTestCaseJ4.java:1576)
        [junit4] > at org.apache.solr.SolrTestCaseJ4.copyMinConf(SolrTestCaseJ4.java:1618)
        [junit4] > at org.apache.solr.SolrTestCaseJ4.copyMinConf(SolrTestCaseJ4.java:1603)
        [junit4] > at org.apache.solr.client.solrj.embedded.MultiCoreExampleJettyTest.testDeleteInstanceDir(MultiCoreExampleJettyTest.java:117)

        You can reproduce the error above with the patch in SOLR-5023

        Show
        Shalin Shekhar Mangar added a comment - Just fyi, the copyMinConf, copyMinFullSetup and copySolrHomeToTemp methods throw the following exception with Solrj tests: junit4] ERROR 0.69s | MultiCoreExampleJettyTest.testDeleteInstanceDir <<< [junit4] > Throwable #1: java.lang.RuntimeException: Cannot find resource: /Users/shalinmangar/work/oss/solr-trunk/solr/build/solr-solrj/test/J0/solr/collection1 [junit4] > at __randomizedtesting.SeedInfo.seed( [2AFBC83FDA207BB2:4160F4A68E96AEF0] :0) [junit4] > at org.apache.solr.SolrTestCaseJ4.getFile(SolrTestCaseJ4.java:1571) [junit4] > at org.apache.solr.SolrTestCaseJ4.TEST_HOME(SolrTestCaseJ4.java:1576) [junit4] > at org.apache.solr.SolrTestCaseJ4.copyMinConf(SolrTestCaseJ4.java:1618) [junit4] > at org.apache.solr.SolrTestCaseJ4.copyMinConf(SolrTestCaseJ4.java:1603) [junit4] > at org.apache.solr.client.solrj.embedded.MultiCoreExampleJettyTest.testDeleteInstanceDir(MultiCoreExampleJettyTest.java:117) You can reproduce the error above with the patch in SOLR-5023
        Hide
        Mark Miller added a comment -

        I suspect that most of the default solr.xml was actually used by tests, and often not by intention...

        Many of the tests were written before solr.xml existed - the built in solr.xml was included for back compat on those pre existing installs. Because it worked with back compat, the people that added solr.xml did not have to update all the tests at the time - something that's commonly done.

        I think that is totally okay though, because we are here to do it now that it actually must be done for 5.0 It's actually probably simpler to deal with now than it was back then.

        Show
        Mark Miller added a comment - I suspect that most of the default solr.xml was actually used by tests, and often not by intention... Many of the tests were written before solr.xml existed - the built in solr.xml was included for back compat on those pre existing installs. Because it worked with back compat, the people that added solr.xml did not have to update all the tests at the time - something that's commonly done. I think that is totally okay though, because we are here to do it now that it actually must be done for 5.0 It's actually probably simpler to deal with now than it was back then.
        Hide
        Erick Erickson added a comment -

        I suspect that most of the default solr.xml was actually used by tests, and often not by intention...

        So the code checked in for 4x is slightly different than the patch for 5 in that it puts back the default solr.xml and includes an explicit test for constructing the default since the point of the other changes is to remove the dependency on the hard-coded solr.xml.

        So going forward, when people write tests they'll have to either
        1> reference a solrhome that contains a solr.xml file
        or
        2> copy the solr.xml file to "the right place". There are several methods in SolrTestCaseJ4 to help here:
        copyMinConf - copies a very tiny schema and config file. Does NOT copy solr.xml
        copyMinFullSetup - as copyMinConf but DOES copy solr.xml
        copySolrHomeToTemp - copies a robust schema and config and solr.xml to a temp dir.

        Show
        Erick Erickson added a comment - I suspect that most of the default solr.xml was actually used by tests, and often not by intention... So the code checked in for 4x is slightly different than the patch for 5 in that it puts back the default solr.xml and includes an explicit test for constructing the default since the point of the other changes is to remove the dependency on the hard-coded solr.xml. So going forward, when people write tests they'll have to either 1> reference a solrhome that contains a solr.xml file or 2> copy the solr.xml file to "the right place". There are several methods in SolrTestCaseJ4 to help here: copyMinConf - copies a very tiny schema and config file. Does NOT copy solr.xml copyMinFullSetup - as copyMinConf but DOES copy solr.xml copySolrHomeToTemp - copies a robust schema and config and solr.xml to a temp dir.
        Hide
        ASF subversion and git services added a comment -

        Commit 1517530 from Erick Erickson in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1517530 ]

        SOLR-4817 Solr should not fall back to the back compat built in solr.xml in SolrCloud mode.

        Show
        ASF subversion and git services added a comment - Commit 1517530 from Erick Erickson in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1517530 ] SOLR-4817 Solr should not fall back to the back compat built in solr.xml in SolrCloud mode.
        Hide
        ASF subversion and git services added a comment -

        Commit 1517398 from Erick Erickson in branch 'dev/trunk'
        [ https://svn.apache.org/r1517398 ]

        SOLR-4817 Solr should not fall back to the back compat built in solr.xml in SolrCloud mode.

        Show
        ASF subversion and git services added a comment - Commit 1517398 from Erick Erickson in branch 'dev/trunk' [ https://svn.apache.org/r1517398 ] SOLR-4817 Solr should not fall back to the back compat built in solr.xml in SolrCloud mode.
        Hide
        Erick Erickson added a comment -

        Final version I think. I'll probably be checking this in this evening. I tried applying it directly to 4x and it works fine, which is not surprising since I changed all the tests to NOT use the default configuration.

        So I'll add another test in to the 4x version to insure we create the default solr.xml when we should after I merge. It turns out that there are only a few small changes to the patch needed for 4x, the aforementioned test and putting the hard-coded bit back in and reverting removing the string from SolrConfigXmlOld.java

        Show
        Erick Erickson added a comment - Final version I think. I'll probably be checking this in this evening. I tried applying it directly to 4x and it works fine, which is not surprising since I changed all the tests to NOT use the default configuration. So I'll add another test in to the 4x version to insure we create the default solr.xml when we should after I merge. It turns out that there are only a few small changes to the patch needed for 4x, the aforementioned test and putting the hard-coded bit back in and reverting removing the string from SolrConfigXmlOld.java
        Hide
        Erick Erickson added a comment -

        Re-worked it in light of lessons learned, it's cleaner now.

        I think it's ready to commit, all tests pass etc.

        Show
        Erick Erickson added a comment - Re-worked it in light of lessons learned, it's cleaner now. I think it's ready to commit, all tests pass etc.
        Hide
        Erick Erickson added a comment -

        Latest patch, all tests pass. That said, I still need to look all the changes over in the morning. I suspect there are some changes that need to be modified re: getting tests to pass But now that all tests pass, perhaps I can be a bit more discriminating in terms of what changes are really necessary.

        In particular, SolrTestCaseJ4.copyTestSolrHome is a nocommit at this point. It's used once, but I'm thinking of leaving it in for the future as a handy method of copying all of a conf directory to a temp dir. It might replace a number of custom copy methods in various tests going forward.

        Show
        Erick Erickson added a comment - Latest patch, all tests pass. That said, I still need to look all the changes over in the morning. I suspect there are some changes that need to be modified re: getting tests to pass But now that all tests pass, perhaps I can be a bit more discriminating in terms of what changes are really necessary. In particular, SolrTestCaseJ4.copyTestSolrHome is a nocommit at this point. It's used once, but I'm thinking of leaving it in for the future as a handy method of copying all of a conf directory to a temp dir. It might replace a number of custom copy methods in various tests going forward.
        Hide
        Erick Erickson added a comment -

        I think this is good-to-go for trunk. Since this really is changing behavior between trunk and 4x I'll put up a separate patch for 4x, I hope today.

        One thing this patch does is completely get rid of the fallback solr.xml string in ConfigSolrXmlOld.xml, it's made available for tests by being put in a test file solr-old-default.xml. The non-test code doesn't have any references to it. This is a stopgap during this period between deprecating persistence and getting rid of it altogether. There's a couple of odd test cases that work if the core container uses this default file but don't work if you use the standard solr.xml file, I don't have the bandwidth to chase that down now, but it'll come out when we remove persistence.

        Show
        Erick Erickson added a comment - I think this is good-to-go for trunk. Since this really is changing behavior between trunk and 4x I'll put up a separate patch for 4x, I hope today. One thing this patch does is completely get rid of the fallback solr.xml string in ConfigSolrXmlOld.xml, it's made available for tests by being put in a test file solr-old-default.xml. The non-test code doesn't have any references to it. This is a stopgap during this period between deprecating persistence and getting rid of it altogether. There's a couple of odd test cases that work if the core container uses this default file but don't work if you use the standard solr.xml file, I don't have the bandwidth to chase that down now, but it'll come out when we remove persistence.
        Hide
        Erick Erickson added a comment -

        Also note I'm still dealing with a couple of failed tests, they don't appear to show up if other tests fail...

        Nothing difficult so far, just a bit of slogging.

        Show
        Erick Erickson added a comment - Also note I'm still dealing with a couple of failed tests, they don't appear to show up if other tests fail... Nothing difficult so far, just a bit of slogging.
        Hide
        Erick Erickson added a comment -

        Alan Woodward Yes, it does remove fallback entirely, but this is for trunk only. The 4x patch will have the fallback in it, which is what's behind my comment about for trunk only and back-compat needing more work for 4x.

        Show
        Erick Erickson added a comment - Alan Woodward Yes, it does remove fallback entirely, but this is for trunk only. The 4x patch will have the fallback in it, which is what's behind my comment about for trunk only and back-compat needing more work for 4x.
        Hide
        Alan Woodward added a comment -

        I may be missing something, but this looks like it removes the 'fall back to built in solr.xml' code entirely? Rather than just in cloud-mode.

        Show
        Alan Woodward added a comment - I may be missing something, but this looks like it removes the 'fall back to built in solr.xml' code entirely? Rather than just in cloud-mode.
        Hide
        Erick Erickson added a comment -

        Patch for trunk only, it'll take a bit of work for back-compat in 4x.

        Alan WoodwardMark Miller I had to do a bit of violence to the persist test and a couple of ZK tests just in case you want to take a quick glance at them.

        All tests passed a bit ago, I'm trying it again now. If that works, I'll see what it would take to make it work with 4x and, if I'm lucky, get it committed over the weekend.

        Show
        Erick Erickson added a comment - Patch for trunk only, it'll take a bit of work for back-compat in 4x. Alan Woodward Mark Miller I had to do a bit of violence to the persist test and a couple of ZK tests just in case you want to take a quick glance at them. All tests passed a bit ago, I'm trying it again now. If that works, I'll see what it would take to make it work with 4x and, if I'm lucky, get it committed over the weekend.
        Hide
        Steve Rowe added a comment -

        Bulk move 4.4 issues to 4.5 and 5.0

        Show
        Steve Rowe added a comment - Bulk move 4.4 issues to 4.5 and 5.0
        Hide
        Hoss Man added a comment -

        +1

        4.4 non/cloud mode: warn that implicit solr.xml is being used.
        4.4 in cloud mode: fail if no solr.xml
        5.0 in either mode: fail if no solr.xml

        Show
        Hoss Man added a comment - +1 4.4 non/cloud mode: warn that implicit solr.xml is being used. 4.4 in cloud mode: fail if no solr.xml 5.0 in either mode: fail if no solr.xml

          People

          • Assignee:
            Erick Erickson
            Reporter:
            Mark Miller
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development