Solr
  1. Solr
  2. SOLR-4790

When defining a core with the same name (discovery mode or not), CoreContainer should throw an error

    Details

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

      Description

      When you define a core with the same name as another core (discovery mode definitely, old-style xml probably), last one wins. Which means it's very hard to track down what caused the problem.

      What's worse, the last-encountered core replaces the first one, leading to cores that change an unexpected index.

      1. SOLR-4790.patch
        9 kB
        Erick Erickson
      2. SOLR-4790.patch
        6 kB
        Erick Erickson
      3. SOLR-4790.patch
        4 kB
        Erick Erickson

        Activity

        Hide
        Erick Erickson added a comment -

        Gaah. Maintaining all the backwards junk is a pain, this is SOOOO much simpler than what was in there before. It only works for discovery mode, I'll take a quick look at what it would take to deal with old-style in a second. If it's too complicated I'll pass on it since old-style is going to end-of-life.

        Anyway, preliminary patch, I'm running the test suite now and have yet to look it over, but is this along the lines you Mark Miller had in mind?

        Show
        Erick Erickson added a comment - Gaah. Maintaining all the backwards junk is a pain, this is SOOOO much simpler than what was in there before. It only works for discovery mode, I'll take a quick look at what it would take to deal with old-style in a second. If it's too complicated I'll pass on it since old-style is going to end-of-life. Anyway, preliminary patch, I'm running the test suite now and have yet to look it over, but is this along the lines you Mark Miller had in mind?
        Hide
        Mark Miller added a comment -

        In this case, rather than a new back compat break, this is really an improvement. In the past, the only legit reason to do this was to reload a core - but now that's a broken way to reload - u must use the reload method. So failing is much better than what we do now IMO.

        Show
        Mark Miller added a comment - In this case, rather than a new back compat break, this is really an improvement. In the past, the only legit reason to do this was to reload a core - but now that's a broken way to reload - u must use the reload method. So failing is much better than what we do now IMO.
        Hide
        Erick Erickson added a comment -

        Updated version, all tests pass. I took a quick look at what happens if you define two cores with the same name old-style, and errors are thrown in addition to the warning issued. So leaving it as-is seems fine.

        But, while looking at that code I think it throws a bogus warning, it just tests for the dataDir defined in the <core> tag. So defining two cores with dataDir=./data would issue the warning even though they were in different instance dirs.

        Two questions:
        1> is
        String absData = new File(instDir, dataDir).getCanonicalPath();
        a reliable way to get the full path? If so, the test in this patch will work in old-style. It worked on a single test anyway.

        2> One could test for duplicate dirs in new-style as well. Is it worth it? Two variants come to mind:
        2a> Pass a map a-la coreDescriptorMap and do simliar stuff.
        2b> in SolrCoreDiscoverer.discover, just analyze the map upon return and issue a warning.

        Either way it would just be some bookeeping local to SolrCoreDiscoverer and not intrusive. But is either one worth the effort?

        [~markrmiller] Andy Fowler any opinions?

        Show
        Erick Erickson added a comment - Updated version, all tests pass. I took a quick look at what happens if you define two cores with the same name old-style, and errors are thrown in addition to the warning issued. So leaving it as-is seems fine. But, while looking at that code I think it throws a bogus warning, it just tests for the dataDir defined in the <core> tag. So defining two cores with dataDir=./data would issue the warning even though they were in different instance dirs. Two questions: 1> is String absData = new File(instDir, dataDir).getCanonicalPath(); a reliable way to get the full path? If so, the test in this patch will work in old-style. It worked on a single test anyway. 2> One could test for duplicate dirs in new-style as well. Is it worth it? Two variants come to mind: 2a> Pass a map a-la coreDescriptorMap and do simliar stuff. 2b> in SolrCoreDiscoverer.discover, just analyze the map upon return and issue a warning. Either way it would just be some bookeeping local to SolrCoreDiscoverer and not intrusive. But is either one worth the effort? [~markrmiller] Andy Fowler any opinions?
        Hide
        Commit Tag Bot added a comment -

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

        SOLR-4790, Defining a core with the same name should throw an error

        Show
        Commit Tag Bot added a comment - [trunk commit] erick http://svn.apache.org/viewvc?view=revision&revision=1481079 SOLR-4790 , Defining a core with the same name should throw an error
        Hide
        Erick Erickson added a comment -

        Final patch

        Show
        Erick Erickson added a comment - Final patch
        Hide
        Commit Tag Bot added a comment -

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

        SOLR-4790, Defining a core with the same name should throw an error

        Show
        Commit Tag Bot added a comment - [branch_4x commit] erick http://svn.apache.org/viewvc?view=revision&revision=1481111 SOLR-4790 , Defining a core with the same name should throw an error
        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:
            Erick Erickson
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development