Solr
  1. Solr
  2. SOLR-5028

Incorrect ShardHandlerFactory creation

    Details

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

      Description

      It seems to me that there are two bugs in the ShardHandlerFactoryCreation that cancel each other and it seems to be working with the old style solr.xml, but not with the "new style". ConfigSolrOldXml seems to be expecting the shardHandlerFactory with the xpath:
      solr/shardHandlerFactory/@class
      Instead of solr/cores/shardHandlerFactory/@class as it used to be. This is never caught because in the CoreContainer the ShardHandlerFactory is initialized using "configSolr.getConfig().getNode("solr/cores/shardHandlerFactory", false);" instead of "configSolr.get(CfgProp.SOLR_SHARDHANDLERFACTORY_CLASS, null);" or something like that. However, if you use the "new style" xml, the CoreContainer will still try to initialize the factory like that, and won't find the SHF.

      1. SOLR-5028.patch
        27 kB
        Alan Woodward
      2. SOLR-5028.patch
        24 kB
        Tomás Fernández Löbbe
      3. SOLR-5028.patch
        20 kB
        Alan Woodward
      4. SOLR-5028.patch
        20 kB
        Alan Woodward
      5. SOLR-5028.patch
        9 kB
        Alan Woodward
      6. SOLR-5082.patch
        5 kB
        Ryan Ernst

        Issue Links

          Activity

          Hide
          Ryan Ernst added a comment -

          Looks like a lot has changed since I fixed this in SOLR-4543...

          The attached patch "fixes" the issue here. I don't understand why the test I had for SolrProperties was not ported over to the "new" xml when the former was removed (added back here).

          But there are still more problems. Persistence right now assumes a flat hierarchy (the addAttrib function takes a single String->String map to fill). This looks like SolrProperties stuff all over again. shardHandlerFactory is a plugin, and it has arbitrary children. Persistence should be getting the Node for shardHandlerFactory and copying it. There should be no need to look for specific children like connTimeout or socketTimeout.

          Show
          Ryan Ernst added a comment - Looks like a lot has changed since I fixed this in SOLR-4543 ... The attached patch "fixes" the issue here. I don't understand why the test I had for SolrProperties was not ported over to the "new" xml when the former was removed (added back here). But there are still more problems. Persistence right now assumes a flat hierarchy (the addAttrib function takes a single String->String map to fill). This looks like SolrProperties stuff all over again. shardHandlerFactory is a plugin, and it has arbitrary children. Persistence should be getting the Node for shardHandlerFactory and copying it. There should be no need to look for specific children like connTimeout or socketTimeout.
          Hide
          Ryan Ernst added a comment -

          I opened another jira for the persistence issue: SOLR-5029

          Show
          Ryan Ernst added a comment - I opened another jira for the persistence issue: SOLR-5029
          Hide
          Tomás Fernández Löbbe added a comment -

          if we take this approach we should remove CfgProp.SOLR_SHARDHANDLERFACTORY_CLASS, SOLR_SHARDHANDLERFACTORY_CONNTIMEOUT, SOLR_SHARDHANDLERFACTORY_NAME, SOLR_SHARDHANDLERFACTORY_SOCKETTIMEOUT so that there is a unique way to access the SHF information. Specially in 4.x where the path to populate those properties is incorrect and using them would result in a bug.

          Show
          Tomás Fernández Löbbe added a comment - if we take this approach we should remove CfgProp.SOLR_SHARDHANDLERFACTORY_CLASS, SOLR_SHARDHANDLERFACTORY_CONNTIMEOUT, SOLR_SHARDHANDLERFACTORY_NAME, SOLR_SHARDHANDLERFACTORY_SOCKETTIMEOUT so that there is a unique way to access the SHF information. Specially in 4.x where the path to populate those properties is incorrect and using them would result in a bug.
          Hide
          Ryan Ernst added a comment -

          I didn't remove those enums only because persistence is using them, and it "sort of" works right now (with HttpShardHandlerFactory, for those specific settings). I leave that to the other jira I opened to fix.

          Show
          Ryan Ernst added a comment - I didn't remove those enums only because persistence is using them, and it "sort of" works right now (with HttpShardHandlerFactory, for those specific settings). I leave that to the other jira I opened to fix.
          Hide
          Tomás Fernández Löbbe added a comment -

          it "sort of" works right now

          not with "old style" solr.xml, there the properties are being loaded with the wrong path:

              propMap.put(CfgProp.SOLR_SHARDHANDLERFACTORY_CLASS,
                  config.getVal("solr/shardHandlerFactory/@class", false));
              propMap.put(CfgProp.SOLR_SHARDHANDLERFACTORY_NAME,
                  config.getVal("solr/shardHandlerFactory/@name", false));
              propMap.put(CfgProp.SOLR_SHARDHANDLERFACTORY_CONNTIMEOUT,
                  config.getVal("solr/shardHandlerFactory/int[@name='connTimeout']", false));
              propMap.put(CfgProp.SOLR_SHARDHANDLERFACTORY_SOCKETTIMEOUT,
                  config.getVal("solr/shardHandlerFactory/int[@name='socketTimeout']", false));
          

          maybe replace those values with

              propMap.put(CfgProp.SOLR_SHARDHANDLERFACTORY_CLASS,
                  config.getVal(getShardHandlerFactoryPath() + "/@class", false));
              ...
          
          Show
          Tomás Fernández Löbbe added a comment - it "sort of" works right now not with "old style" solr.xml, there the properties are being loaded with the wrong path: propMap.put(CfgProp.SOLR_SHARDHANDLERFACTORY_CLASS, config.getVal( "solr/shardHandlerFactory/@class" , false )); propMap.put(CfgProp.SOLR_SHARDHANDLERFACTORY_NAME, config.getVal( "solr/shardHandlerFactory/@name" , false )); propMap.put(CfgProp.SOLR_SHARDHANDLERFACTORY_CONNTIMEOUT, config.getVal( "solr/shardHandlerFactory/ int [@name='connTimeout']" , false )); propMap.put(CfgProp.SOLR_SHARDHANDLERFACTORY_SOCKETTIMEOUT, config.getVal( "solr/shardHandlerFactory/ int [@name='socketTimeout']" , false )); maybe replace those values with propMap.put(CfgProp.SOLR_SHARDHANDLERFACTORY_CLASS, config.getVal(getShardHandlerFactoryPath() + "/@class" , false )); ...
          Hide
          Erick Erickson added a comment -

          Tomás: Don't quite get this. Those enums are just for use as key values in the map, not for the paths so I think they should stay.

          But getting the values to assign to the maps keyed by things like CfgProp.SOLR_SHARDHANDLERFACTORY_SOCKETTIMEOUT by the getShardHandlerFactoryPath() trick is a different matter.

          Show
          Erick Erickson added a comment - Tomás: Don't quite get this. Those enums are just for use as key values in the map, not for the paths so I think they should stay. But getting the values to assign to the maps keyed by things like CfgProp.SOLR_SHARDHANDLERFACTORY_SOCKETTIMEOUT by the getShardHandlerFactoryPath() trick is a different matter.
          Hide
          Alan Woodward added a comment -

          A couple of tweaks to Ryan's patch, moving the creation logic into a static method on ShardHandlerInfo and adding getShardHandlerFactoryPluginInfo() methods to the ConfigSolr implementations. Lovely lovely Java car-crash class names...

          Ryan, Tomás, could you try this out? Will work on a persistence patch next.

          Show
          Alan Woodward added a comment - A couple of tweaks to Ryan's patch, moving the creation logic into a static method on ShardHandlerInfo and adding getShardHandlerFactoryPluginInfo() methods to the ConfigSolr implementations. Lovely lovely Java car-crash class names... Ryan, Tomás, could you try this out? Will work on a persistence patch next.
          Hide
          Alan Woodward added a comment -

          Patch with persistence added.

          Show
          Alan Woodward added a comment - Patch with persistence added.
          Hide
          Erick Erickson added a comment -

          Alan:

          I think the tag for 4.4 has been cut (check with sarowe), so when you merge this in to 4.x don't forget to add it into the 4.4 branch too.

          Show
          Erick Erickson added a comment - Alan: I think the tag for 4.4 has been cut (check with sarowe), so when you merge this in to 4.x don't forget to add it into the 4.4 branch too.
          Hide
          Tomás Fernández Löbbe added a comment -

          Tomás: Don't quite get this. Those enums are just for use as key values in the map, not for the paths so I think they should stay.

          Yes, I know, but what I'm saying is that as the SHF receives an unknown set of parameters (because it's a plugin) you can't generate enum keys for everything. Instead of that, when someone needs to get information about the SHF configuration it'll need to get the Node (or the PluginInfo with Alan's patch). "connTimeout" and "socketTimeout" are arguments specific to the HttpShardHandlerFactory that other SHF implementations may or may not use. Also, HttpShardHandlerFactory accepts lots of other arguments and we are not providing keys for all of them. So it gets confusing, some arbitrary init params can be get using cfg.get(...) and some can't.

          Ryan, Tomás, could you try this out? Will work on a persistence patch next.

          I haven't tested it yet, but +1 on the changes you did. ConfigSolrXmlOld still populates the map with the wrong paths though.

          Show
          Tomás Fernández Löbbe added a comment - Tomás: Don't quite get this. Those enums are just for use as key values in the map, not for the paths so I think they should stay. Yes, I know, but what I'm saying is that as the SHF receives an unknown set of parameters (because it's a plugin) you can't generate enum keys for everything. Instead of that, when someone needs to get information about the SHF configuration it'll need to get the Node (or the PluginInfo with Alan's patch). "connTimeout" and "socketTimeout" are arguments specific to the HttpShardHandlerFactory that other SHF implementations may or may not use. Also, HttpShardHandlerFactory accepts lots of other arguments and we are not providing keys for all of them. So it gets confusing, some arbitrary init params can be get using cfg.get(...) and some can't. Ryan, Tomás, could you try this out? Will work on a persistence patch next. I haven't tested it yet, but +1 on the changes you did. ConfigSolrXmlOld still populates the map with the wrong paths though.
          Hide
          Erick Erickson added a comment -

          Tomás:

          Got it, thanks. Right, removing them is a Good Thing then.

          No one will be happier than me when we get rid of this persistence stuff. I hope I made it better with the first go-round. I'm sure Alan's made it better with his refactoring. And it's still very hard to get everything right.

          Show
          Erick Erickson added a comment - Tomás: Got it, thanks. Right, removing them is a Good Thing then. No one will be happier than me when we get rid of this persistence stuff. I hope I made it better with the first go-round. I'm sure Alan's made it better with his refactoring. And it's still very hard to get everything right.
          Hide
          Alan Woodward added a comment -

          TBH, it would be nicest to just remove cfg.get() entirely, and make ConfigSolr a proper POJO. But that's another issue.

          This is still not ideal, as I've just noticed that my quick-n-dirty patch still assumes a single-level plugin info hierarchy and won't deal with <arr> or <lst> values properly. Will try and fix that.

          Show
          Alan Woodward added a comment - TBH, it would be nicest to just remove cfg.get() entirely, and make ConfigSolr a proper POJO. But that's another issue. This is still not ideal, as I've just noticed that my quick-n-dirty patch still assumes a single-level plugin info hierarchy and won't deal with <arr> or <lst> values properly. Will try and fix that.
          Hide
          Alan Woodward added a comment -

          The first rule of XML processing is: never try and write your own XML processor.

          Updated patch, using dom Transformers to write out the plugin info Node. Tests pass, I think this is good to commit.

          Show
          Alan Woodward added a comment - The first rule of XML processing is: never try and write your own XML processor. Updated patch, using dom Transformers to write out the plugin info Node. Tests pass, I think this is good to commit.
          Hide
          Tomás Fernández Löbbe added a comment -

          I insist that the SHF properties in ConfigSolr should either be fixed or removed. I attach a patch removing them.

          Also, your patch seems to miss the file solr-shardhandler-old.xml, adding one.

          Show
          Tomás Fernández Löbbe added a comment - I insist that the SHF properties in ConfigSolr should either be fixed or removed. I attach a patch removing them. Also, your patch seems to miss the file solr-shardhandler-old.xml, adding one.
          Hide
          Alan Woodward added a comment -

          Agreed, Tomás. Final patch, with CHANGES entry as well. Committing shortly.

          Show
          Alan Woodward added a comment - Agreed, Tomás. Final patch, with CHANGES entry as well. Committing shortly.
          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
          Hide
          Alan Woodward added a comment -

          Thanks all.

          Show
          Alan Woodward added a comment - Thanks all.
          Hide
          Ryan Ernst added a comment -

          Alan this is great, thanks so much!

          Show
          Ryan Ernst added a comment - Alan this is great, thanks so much!
          Hide
          Alan Woodward added a comment -

          No worries - it was lucky you raised this when you did, as I was about to completely screw it up by rewriting all the persistence logic

          Show
          Alan Woodward added a comment - No worries - it was lucky you raised this when you did, as I was about to completely screw it up by rewriting all the persistence logic
          Hide
          Jack Krupansky added a comment -

          Does this issue have a fix version? Like, 4.5?

          Show
          Jack Krupansky added a comment - Does this issue have a fix version? Like, 4.5?
          Hide
          Adrien Grand added a comment -

          4.5 release -> bulk close

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

            People

            • Assignee:
              Alan Woodward
              Reporter:
              Tomás Fernández Löbbe
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development