Solr
  1. Solr
  2. SOLR-5029

shardHandlerFactory is not properly persisted

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      In SOLR-5028 I discovered persistence for shardHandlerFactory is only looking for connTimeout and socketTimeout children. Persistence should work for any SHF impl, not just HttpShardHandlerFactory. I think the thing to do here is just copying the underlying Node to the new file, but the current persistence code assumes a flat (String->String) hierarchy (which seems wrong, flat hierarchy was one of the reasons myself and others were against solr.properties).

        Issue Links

          Activity

          Hide
          Alan Woodward added a comment -

          Hi Ryan, SOLR-4914 should fix this. The only thing that actually changes in solr.xml is the cores list, so I've got round all the horribly complicated persistence rules by just storing most of the XML as a plain string, and interpolating the core information back in at persistence time.

          Show
          Alan Woodward added a comment - Hi Ryan, SOLR-4914 should fix this. The only thing that actually changes in solr.xml is the cores list, so I've got round all the horribly complicated persistence rules by just storing most of the XML as a plain string, and interpolating the core information back in at persistence time.
          Hide
          Alan Woodward added a comment -

          I think this isn't being caught because the example config in TestSolrXmlPersistence has shardHandlerFactory defined in the wrong place (directly below solr/, rather than below solr/cores). And SOLR-4914 will also break this, because I was assuming that only <core> tags appeared below <cores>, and none of the tests picked up on it.

          Show
          Alan Woodward added a comment - I think this isn't being caught because the example config in TestSolrXmlPersistence has shardHandlerFactory defined in the wrong place (directly below solr/, rather than below solr/cores). And SOLR-4914 will also break this, because I was assuming that only <core> tags appeared below <cores>, and none of the tests picked up on it.
          Hide
          Erick Erickson added a comment -

          Oh dear. It's worse than that. Both ConfigSolr.xml and ConfigSolrOld.xml look for the shard definition directly under solr. CoreContainer.initShardHandler looks under solr/cores.

          Ugly as the idea is, how about this? Support both positions (which will require initShardHandler looking in both places?) for 4.x. Only support the <shardHandlerFactory> as an immediate child node to <solr> in 5.0. And persist it to an immediate child of <solr> for 4.x which would, indeed, move it.

          Show
          Erick Erickson added a comment - Oh dear. It's worse than that. Both ConfigSolr.xml and ConfigSolrOld.xml look for the shard definition directly under solr. CoreContainer.initShardHandler looks under solr/cores. Ugly as the idea is, how about this? Support both positions (which will require initShardHandler looking in both places?) for 4.x. Only support the <shardHandlerFactory> as an immediate child node to <solr> in 5.0. And persist it to an immediate child of <solr> for 4.x which would, indeed, move it.
          Hide
          Ryan Ernst added a comment -

          Erick, did you look at the related SOLR-5028? I already have a patch fixing the issue of SHF construction for the "new xml" style there.

          Show
          Ryan Ernst added a comment - Erick, did you look at the related SOLR-5028 ? I already have a patch fixing the issue of SHF construction for the "new xml" style there.
          Hide
          Erick Erickson added a comment -

          Ryan:

          Nope, didn't see that. I think the fixes aren't all that hard if we can agree on what they should be.

          I see what you're trying to do in SOLR-5028, but I don't think that handles Alan's persistence simplification. Of course that wasn't there until recently. Which is pretty much what was behind the proposal to move it automagically for 4.x to a child of <solr>. But I'd like Alan to weigh in on it....

          For that matter, I don't think the current checked-in code for persistence does the right thing in both cases either.

          Alan, how do you think we should reconcile all this?

          Show
          Erick Erickson added a comment - Ryan: Nope, didn't see that. I think the fixes aren't all that hard if we can agree on what they should be. I see what you're trying to do in SOLR-5028 , but I don't think that handles Alan's persistence simplification. Of course that wasn't there until recently. Which is pretty much what was behind the proposal to move it automagically for 4.x to a child of <solr>. But I'd like Alan to weigh in on it.... For that matter, I don't think the current checked-in code for persistence does the right thing in both cases either. Alan, how do you think we should reconcile all this?
          Hide
          Alan Woodward added a comment -

          I think we need to get this fixed for 4.4, and worry about how we reconcile it with SOLR-4914 later...

          My suggestion:

          • add an abstract getShardHandlerFactoryPluginInfo() to ConfigSolr which returns the relevant PluginInfo, moving all the 'where do we get it' logic into the two different ConfigSolrXXXX implementations. Use this in initShardHandler().
          • pass this PluginInfo to SolrCores.persistCores() instead of the shardHandlerAttrib map.

          I'll put together a patch.

          Show
          Alan Woodward added a comment - I think we need to get this fixed for 4.4, and worry about how we reconcile it with SOLR-4914 later... My suggestion: add an abstract getShardHandlerFactoryPluginInfo() to ConfigSolr which returns the relevant PluginInfo, moving all the 'where do we get it' logic into the two different ConfigSolrXXXX implementations. Use this in initShardHandler(). pass this PluginInfo to SolrCores.persistCores() instead of the shardHandlerAttrib map. I'll put together a patch.
          Hide
          Alan Woodward added a comment -

          Patch put up on SOLR-5028, which I think should fix this for 4.4. If we're happy with that, then I can start on updating SOLR-4914 to deal with all of this stuff.

          Show
          Alan Woodward added a comment - Patch put up on SOLR-5028 , which I think should fix this for 4.4. If we're happy with that, then I can start on updating SOLR-4914 to deal with all of this stuff.
          Hide
          ASF subversion and git services added a comment -

          Commit 1502231 from Alan Woodward
          [ https://svn.apache.org/r1502231 ]

          SOLR-5028,SOLR-5029: Fix ShardHandlerFactory creation and persistence

          Show
          ASF subversion and git services added a comment - Commit 1502231 from Alan Woodward [ https://svn.apache.org/r1502231 ] SOLR-5028 , SOLR-5029 : Fix ShardHandlerFactory creation and persistence
          Hide
          ASF subversion and git services added a comment -

          Commit 1502233 from Alan Woodward
          [ https://svn.apache.org/r1502233 ]

          SOLR-5028,SOLR-5029: Fix ShardHandlerFactory creation and persistence

          Show
          ASF subversion and git services added a comment - Commit 1502233 from Alan Woodward [ https://svn.apache.org/r1502233 ] SOLR-5028 , SOLR-5029 : Fix ShardHandlerFactory creation and persistence
          Hide
          ASF subversion and git services added a comment -

          Commit 1502235 from Alan Woodward
          [ https://svn.apache.org/r1502235 ]

          SOLR-5028,SOLR-5029: Fix ShardHandlerFactory creation and persistence

          Show
          ASF subversion and git services added a comment - Commit 1502235 from Alan Woodward [ https://svn.apache.org/r1502235 ] SOLR-5028 , SOLR-5029 : Fix ShardHandlerFactory creation and persistence

            People

            • Assignee:
              Unassigned
              Reporter:
              Ryan Ernst
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development