Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.3
    • Component/s: None
    • Labels:
      None

      Description

      Per mailing list discussion (see link below) it seems prudent to rename multicore.xml to solr.xml prior to the 1.3 release.

      short summary of the main motivations for doing this...

         1) The name of the file corresponds with one specific way it can be
            used, but may not be applicable to all usages (namely: you can
            have a multicore.xml file but only use one core)
         2) The "first" config file checked to determine the application's
            behavior, and what paths will be checked for other config files
            is named after one specific feature of the application. 
      
      

      General consensus of the thread so far seems to be that this is a good idea, and gives us more options for the future.

      http://www.nabble.com/Rename-multicore.xml---to18877268.html

      1. SOLR-689.patch
        9 kB
        Hoss Man
      2. SOLR-689.patch
        7 kB
        Hoss Man

        Issue Links

          Activity

          Hide
          Hoss Man added a comment -

          Patch represents the minimal things needed to accomplish the following:

          1. example "Multi Core" config is renamed solr.xml
          2. example solr.xml has root tag of <solr> containing <multicore>
          3. MultiCore.java accepts root tag of <solr>, but does not require it .. only cares that <multicore> exists somewhere
          4. MultiCore.java config persistence writes info out to solr.xml and includes <solr> root tag
          5. references to multicore.xml in CHANGES.txt changed to solr.xml
          6. all tests pass

          Things not attempted in this patch:

          1. renaming or refactoring existing methods or classes
          2. renaming config files used by tests from multicore*.xml to solr*.xml
          3. adding root <solr> tag to config files used by tests

          (those things can still be done, but are less crucial in my opinion)

          Comments? Objections to this being committed?

          Show
          Hoss Man added a comment - Patch represents the minimal things needed to accomplish the following: example "Multi Core" config is renamed solr.xml example solr.xml has root tag of <solr> containing <multicore> MultiCore.java accepts root tag of <solr>, but does not require it .. only cares that <multicore> exists somewhere MultiCore.java config persistence writes info out to solr.xml and includes <solr> root tag references to multicore.xml in CHANGES.txt changed to solr.xml all tests pass Things not attempted in this patch: renaming or refactoring existing methods or classes renaming config files used by tests from multicore*.xml to solr*.xml adding root <solr> tag to config files used by tests (those things can still be done, but are less crucial in my opinion) Comments? Objections to this being committed?
          Hide
          Hoss Man added a comment -

          PS: the usual caveats about committing patches that involve file renaming apply to this patch. care should be taken when applying it to ensure that the subsequent commit includes both the file history of "example/multicore/multicore.xml" as well as the changes that were made to it after it was renamed.

          Show
          Hoss Man added a comment - PS: the usual caveats about committing patches that involve file renaming apply to this patch. care should be taken when applying it to ensure that the subsequent commit includes both the file history of "example/multicore/multicore.xml" as well as the changes that were made to it after it was renamed.
          Hide
          Ryan McKinley added a comment -

          +1

          whats up with the // bit in:

          -      persistent = cfg.getBool( "multicore/@persistent", false );
          -      adminPath  = cfg.get(     "multicore/@adminPath", null );
          -      libDir     = cfg.get(     "multicore/@sharedLib", null);
          +      persistent = cfg.getBool( "//multicore/@persistent", false );
          +      adminPath  = cfg.get(     "//multicore/@adminPath", null );
          +      libDir     = cfg.get(     "//multicore/@sharedLib", null);
          

          is that just safer since we may not be at the root?

          Show
          Ryan McKinley added a comment - +1 whats up with the // bit in: - persistent = cfg.getBool( "multicore/@persistent" , false ); - adminPath = cfg.get( "multicore/@adminPath" , null ); - libDir = cfg.get( "multicore/@sharedLib" , null ); + persistent = cfg.getBool( " //multicore/@persistent" , false ); + adminPath = cfg.get( " //multicore/@adminPath" , null ); + libDir = cfg.get( " //multicore/@sharedLib" , null ); is that just safer since we may not be at the root?
          Hide
          Hoss Man added a comment -

          it's not just safer, the old xpath expressions won't work with <solr> as the root node ... they are evaluated at the document level so "multicore/@persistent" requires that the root be <multicore>

          i could have changed them to "solr/multicore/..." but this way if someone already has a multicore.xml file all they need to do is rename it and it will still work.

          (i suppose we could output a warning if it's not rooted with <solr> but for now i wanted to keep the patch as simple as possible to meet the immediate goals)

          Show
          Hoss Man added a comment - it's not just safer, the old xpath expressions won't work with <solr> as the root node ... they are evaluated at the document level so "multicore/@persistent" requires that the root be <multicore> i could have changed them to "solr/multicore/..." but this way if someone already has a multicore.xml file all they need to do is rename it and it will still work. (i suppose we could output a warning if it's not rooted with <solr> but for now i wanted to keep the patch as simple as possible to meet the immediate goals)
          Hide
          Ryan McKinley added a comment -

          Since we are already throwing a wrench at anyone who has an existing multicore.xml file, I think we should go ahead and make the other name changes:

          <multicore adminPath="/admin/multicore" persistent="true" >
            <core name="core0" instanceDir="core0" />
            <core name="core1" instanceDir="core1" />
          </multicore>
          

          to

          <solr coreAdminHandler="/admin/multicore" persistent="true" >
            <core name="core0" instanceDir="core0" />
            <core name="core1" instanceDir="core1" />
          </solr>
          

          I agree that internal class refactoring can change later (or sooner), but if we muck with the external configs, we should do everything at once.

          Show
          Ryan McKinley added a comment - Since we are already throwing a wrench at anyone who has an existing multicore.xml file, I think we should go ahead and make the other name changes: <multicore adminPath= "/admin/multicore" persistent= "true" > <core name= "core0" instanceDir= "core0" /> <core name= "core1" instanceDir= "core1" /> </multicore> to <solr coreAdminHandler= "/admin/multicore" persistent= "true" > <core name= "core0" instanceDir= "core0" /> <core name= "core1" instanceDir= "core1" /> </solr> I agree that internal class refactoring can change later (or sooner), but if we muck with the external configs, we should do everything at once.
          Hide
          Hoss Man added a comment -

          to be clear: with this patch as it stands, the current syntax, with all supported options, is...

          <solr>
            <multicore adminPath="/admin/multicore" persistent="true" sharedLib="/shared/lib">
              <core name="core0" instanceDir="core0" />
              <core name="core1" instanceDir="core1" />
            </multicore>
          </solr>
          

          moving "persistent" to the <solr> tag makes sense, it's impossible for it to be specific only to the <multicore> block.

          renaming "adminPath" to "coreAdminHandler" makes sense ... i guess ... but i would think "coreAdminPath" or "coreAdmin" or "adminPath" would still be better vernacular - we're not asking people to specify a handler, we're asking them to specify a path. The fact that it's a handler isn't something they even need to know about, all they need to know is that this is the relative URL for administer cores.

          removing <multicore> gives me pause ... as a bit of future proofing it seems like it would make sense to collect the list of cores into a common parent (so if, for some reason, we add other stuff unrelated to the list of cores there is a clear delineation. <cores> perhaps?

          How about...

          <solr persistent="true" sharedLib="/shared/lib">
            <cores adminPath="/admin/multicore" >
              <core name="core0" instanceDir="core0" />
              <core name="core1" instanceDir="core1" />
            </cores>
          </solr>
          

          ?

          Show
          Hoss Man added a comment - to be clear: with this patch as it stands, the current syntax, with all supported options, is... <solr> <multicore adminPath= "/admin/multicore" persistent= " true " sharedLib= "/shared/lib" > <core name= "core0" instanceDir= "core0" /> <core name= "core1" instanceDir= "core1" /> </multicore> </solr> moving "persistent" to the <solr> tag makes sense, it's impossible for it to be specific only to the <multicore> block. renaming "adminPath" to "coreAdminHandler" makes sense ... i guess ... but i would think "coreAdminPath" or "coreAdmin" or "adminPath" would still be better vernacular - we're not asking people to specify a handler, we're asking them to specify a path. The fact that it's a handler isn't something they even need to know about, all they need to know is that this is the relative URL for administer cores. removing <multicore> gives me pause ... as a bit of future proofing it seems like it would make sense to collect the list of cores into a common parent (so if, for some reason, we add other stuff unrelated to the list of cores there is a clear delineation. <cores> perhaps? How about... <solr persistent= " true " sharedLib= "/shared/lib" > <cores adminPath= "/admin/multicore" > <core name= "core0" instanceDir= "core0" /> <core name= "core1" instanceDir= "core1" /> </cores> </solr> ?
          Hide
          Ryan McKinley added a comment -

          There you go... always looking towards the future

          Yes, i like that – keeping adminPath with <cores> makes sense too

          Show
          Ryan McKinley added a comment - There you go... always looking towards the future Yes, i like that – keeping adminPath with <cores> makes sense too
          Hide
          Koji Sekiguchi added a comment - - edited

          MultiCore.java config persistence writes info out to solr.xml and includes <solr> root tag

          writer.write("</solr>\n");
          writer.write("</multicore>\n");
          

          should be:

          writer.write("</multicore>\n");
          writer.write("</solr>\n");
          

          Show
          Koji Sekiguchi added a comment - - edited MultiCore.java config persistence writes info out to solr.xml and includes <solr> root tag writer.write( "</solr>\n" ); writer.write( "</multicore>\n" ); should be: writer.write( "</multicore>\n" ); writer.write( "</solr>\n" );
          Hide
          Henri Biestro added a comment -

          Regarding this issue wrt solr-646, what would be the preferred syntax to declare properties:
          Shoud we have a <properties>...</properties> tag to declare them in solr & in core or only in solr ?
          How about the following example:

          <solr persistent="true" sharedLib="/shared/lib">
            <properties>
              <property name="even">core-122</property>
              <property name="odd">core-123</property>
            </properties>
            <cores adminPath="/admin/multicore" >
              <core name="prod" instanceDir="${even}" >
                <property name='updateHandler'>solr.DirectUpdateHandler2</property>
              </core>
              <core name="tmp" instanceDir="${odd}">
                <property name='updateHandler'>solr.DirectUpdateHandler2</property>
              </core>
            </cores>
          </solr>
          
          Show
          Henri Biestro added a comment - Regarding this issue wrt solr-646, what would be the preferred syntax to declare properties: Shoud we have a <properties>...</properties> tag to declare them in solr & in core or only in solr ? How about the following example: <solr persistent= "true" sharedLib= "/shared/lib" > <properties> <property name= "even" > core-122 </property> <property name= "odd" > core-123 </property> </properties> <cores adminPath= "/admin/multicore" > <core name= "prod" instanceDir= "${even}" > <property name='updateHandler'> solr.DirectUpdateHandler2 </property> </core> <core name= "tmp" instanceDir= "${odd}" > <property name='updateHandler'> solr.DirectUpdateHandler2 </property> </core> </cores> </solr>
          Hide
          Hoss Man added a comment - - edited

          Patch updated to reflect recent discussion and fix bug Henri (Koji) noticed about generating completely invalid XML in the persist method. – we clearly don't have anything testing that.

          I'd like to commit this tonight (in about 8 or 9 hours).

          Before i do: would someone who knows more about the "multicore persistence" mind writing a test for that? It can be, and probably should be, independent from this patch, but if you attach it here i'll commit as well

          Show
          Hoss Man added a comment - - edited Patch updated to reflect recent discussion and fix bug Henri (Koji) noticed about generating completely invalid XML in the persist method. – we clearly don't have anything testing that. I'd like to commit this tonight (in about 8 or 9 hours). Before i do: would someone who knows more about the "multicore persistence" mind writing a test for that? It can be, and probably should be, independent from this patch, but if you attach it here i'll commit as well
          Hide
          Hoss Man added a comment -

          Regarding this issue wrt solr-646, what would be the preferred syntax to declare properties:

          Henri: I don't think anything in this issue really impacts the semantics of SOLR-646, it's a question of intent: if the intent is to declare properties that are valid anywhere in the file, then they should be children of the <solr> tag. if the intent is to create properties that apply to all cores, but can't be used elsewhere in the solr.xml, they should be in the <cores> tag. we could have both, or just one depending on the use cases people have identified.

          Show
          Hoss Man added a comment - Regarding this issue wrt solr-646, what would be the preferred syntax to declare properties: Henri: I don't think anything in this issue really impacts the semantics of SOLR-646 , it's a question of intent: if the intent is to declare properties that are valid anywhere in the file, then they should be children of the <solr> tag. if the intent is to create properties that apply to all cores, but can't be used elsewhere in the solr.xml, they should be in the <cores> tag. we could have both, or just one depending on the use cases people have identified.
          Hide
          Henri Biestro added a comment -

          HossMan; I understand, my question was just syntactical.
          The patch today does accomodate properties created at the solr (aka multicore) & core levels (which allows a core to override "global" defaults) .
          I just wanted to know if there were preferences regarding "grouping" them under a "properties" tag or not. (Much like having <cores> or not to group cores) so I could prepare a new version of the solr-646 patch (and add a persist test including properties).

          Show
          Henri Biestro added a comment - HossMan; I understand, my question was just syntactical. The patch today does accomodate properties created at the solr (aka multicore) & core levels (which allows a core to override "global" defaults) . I just wanted to know if there were preferences regarding "grouping" them under a "properties" tag or not. (Much like having <cores> or not to group cores) so I could prepare a new version of the solr-646 patch (and add a persist test including properties).
          Hide
          Hoss Man added a comment - - edited

          I just wanted to know if there were preferences regarding "grouping" them under a "properties" tag or not. (Much like having <cores> or not to group cores)

          i don't really have an opinion on that ... to my mind: <properties> isn't needed ... grouping multiple<core> tags into <cores> makes sense to segregate their configuration from anything else that might go in the file (particularly since the adminPath is specific to the <cores>) ... the <property> declarations would be applicable to whatever context they are in right? so introducing a new <properties> context would be ... odd.

          I could prepare a new version of the solr-646 patch (and add a persist test including properties).

          I'm not sure what the time frame is for SOLR-646, but i ment it would be nice to have a persistence test that worked against the trunk as it is right now, to prove that the behavior doesn't break when the SOLR-689.patch is applied. (ie: commit a persistence test first, then commit SOLR-689.patch; worry about SOLR-646 and any changes it makes separately)

          Show
          Hoss Man added a comment - - edited I just wanted to know if there were preferences regarding "grouping" them under a "properties" tag or not. (Much like having <cores> or not to group cores) i don't really have an opinion on that ... to my mind: <properties> isn't needed ... grouping multiple<core> tags into <cores> makes sense to segregate their configuration from anything else that might go in the file (particularly since the adminPath is specific to the <cores>) ... the <property> declarations would be applicable to whatever context they are in right? so introducing a new <properties> context would be ... odd. I could prepare a new version of the solr-646 patch (and add a persist test including properties). I'm not sure what the time frame is for SOLR-646 , but i ment it would be nice to have a persistence test that worked against the trunk as it is right now, to prove that the behavior doesn't break when the SOLR-689 .patch is applied. (ie: commit a persistence test first, then commit SOLR-689 .patch; worry about SOLR-646 and any changes it makes separately)
          Hide
          Hoss Man added a comment -

          Committed revision 685244.

          Show
          Hoss Man added a comment - Committed revision 685244.

            People

            • Assignee:
              Hoss Man
              Reporter:
              Hoss Man
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development