Solr
  1. Solr
  2. SOLR-4615

Take out the possibility of having a solr.properties file

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 4.3, 5.0
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      We seem to have re-thought whether deprecating Solr.xml is The Right Thing To Do or not, the consensus seems to be that we should keep solr.xml, not be able to specify solr.properties but add an attribute to the <cores> tag in solr.xml, tentatively called autoDiscover=true|false (assume false for 4.x, true for 5.0?)

      This really has to be done before 4.3 is cut, as in Real Soon Now.

      1. SOLR-4615.patch
        100 kB
        Erick Erickson
      2. SOLR-4615.patch
        103 kB
        Erick Erickson

        Issue Links

          Activity

          Hide
          Mark Miller added a comment -

          I actually think the current format is not right - almost everything is an attribute in <cores> tag. That seems less than ideal...I wonder if we could just design the solr.xml from the ground up - as if it was a properties file. I have not donated any sweat to this yet, so just an idea, but I think we can do better than the current solr.xml - <cores>.

          Show
          Mark Miller added a comment - I actually think the current format is not right - almost everything is an attribute in <cores> tag. That seems less than ideal...I wonder if we could just design the solr.xml from the ground up - as if it was a properties file. I have not donated any sweat to this yet, so just an idea, but I think we can do better than the current solr.xml - <cores>.
          Hide
          Jan Høydahl added a comment -

          That reminds me that I did just that in SOLR-3613 - refactored solr.xml into a better structure for the cloud params. But it was never completed due to arguments about not wanting to namespace cloud java-options for the 4.0.0 release due to back-compat with beta.

          I just uploaded my work to that patch, it won't apply to today's trunk, but have a look at the proposed solr.xml format. The patch tries to prefix all params such as zkHost with a solr. prefix, but still keep the short form as a valid alias. But then I got somewhat lost trying to also support webapp-name. prefixing automatically...

          Show
          Jan Høydahl added a comment - That reminds me that I did just that in SOLR-3613 - refactored solr.xml into a better structure for the cloud params. But it was never completed due to arguments about not wanting to namespace cloud java-options for the 4.0.0 release due to back-compat with beta. I just uploaded my work to that patch, it won't apply to today's trunk, but have a look at the proposed solr.xml format. The patch tries to prefix all params such as zkHost with a solr. prefix, but still keep the short form as a valid alias. But then I got somewhat lost trying to also support webapp-name. prefixing automatically...
          Hide
          Erick Erickson added a comment -

          I had a long plane ride so I cut solr.properties out in the attached patch. All tests pass, although I haven't yet really gone over it yet, consider this preliminary.

          Known tasks not yet done:
          1> add tests for persisting the new property.
          2> there are a couple of JIRAs (SOLR-4631 and SOLR-4632) that I have yet to incorporate.

          There is a new attribute for <cores>, autoDiscoverCores=["true"|"false"], defaults presently to "false". If you'd like a different name, speak now...

          I left the interface definition in place, currently only ConfigSolrXml implements it (renamed from ConfigSolrXmlBackCompat). My theory is that since it's there already, leaving it in has the advantage of allowing easier implementation of a pluggable configuration provider of some kind should the desire arise. One possibility (although I know nothing about whether it's desirable) would be provide all this info from ZK directly, maybe based on a sysprop?

          I also renamed several files, removed the supporting config fies for solr.properties etc., so be aware that some things have moved around...

          I plan to commit this in the next couple of days to both trunk and 4x on the theory that the faster I take solr.properties out of code anyone can get to, the less they'll be inconvenienced by flip-flopping.

          Changing this didn't require any changes to the tricky bits around coordinating core load/unload/reload code, so it's not a radical change. Not to say I necessarily did it right, but it's not nearly as scary as the rest of the changes for SOLR-4196.

          All comments welcome....

          Show
          Erick Erickson added a comment - I had a long plane ride so I cut solr.properties out in the attached patch. All tests pass, although I haven't yet really gone over it yet, consider this preliminary. Known tasks not yet done: 1> add tests for persisting the new property. 2> there are a couple of JIRAs ( SOLR-4631 and SOLR-4632 ) that I have yet to incorporate. There is a new attribute for <cores>, autoDiscoverCores= ["true"|"false"] , defaults presently to "false". If you'd like a different name, speak now... I left the interface definition in place, currently only ConfigSolrXml implements it (renamed from ConfigSolrXmlBackCompat). My theory is that since it's there already, leaving it in has the advantage of allowing easier implementation of a pluggable configuration provider of some kind should the desire arise. One possibility (although I know nothing about whether it's desirable) would be provide all this info from ZK directly, maybe based on a sysprop? I also renamed several files, removed the supporting config fies for solr.properties etc., so be aware that some things have moved around... I plan to commit this in the next couple of days to both trunk and 4x on the theory that the faster I take solr.properties out of code anyone can get to, the less they'll be inconvenienced by flip-flopping. Changing this didn't require any changes to the tricky bits around coordinating core load/unload/reload code, so it's not a radical change. Not to say I necessarily did it right, but it's not nearly as scary as the rest of the changes for SOLR-4196 . All comments welcome....
          Hide
          Erick Erickson added a comment -

          Jan:

          Don't quite have the energy to tackle SOLR-3613, but I'd be glad to comment on the sidelines....

          Show
          Erick Erickson added a comment - Jan: Don't quite have the energy to tackle SOLR-3613 , but I'd be glad to comment on the sidelines....
          Hide
          Mark Miller added a comment -

          autoDiscoverCores=["true"|"false"]

          I'm not a fan of this - I don't believe false should be an option in the future, so I don't like adding this option. Rather, we should just determine if it's an old solr.xml or a new one (add a version, change the format, change the file name, something - I'll have to dig in first to know what I would vote for).

          Given that, I also still think we should change the solr.xml format to make more sense. I'll be pitching in on this soon (I've said that for a while, but I keep getting caught up in other things - I will be doing this before 4.3 though.)

          Show
          Mark Miller added a comment - autoDiscoverCores= ["true"|"false"] I'm not a fan of this - I don't believe false should be an option in the future, so I don't like adding this option. Rather, we should just determine if it's an old solr.xml or a new one (add a version, change the format, change the file name, something - I'll have to dig in first to know what I would vote for). Given that, I also still think we should change the solr.xml format to make more sense. I'll be pitching in on this soon (I've said that for a while, but I keep getting caught up in other things - I will be doing this before 4.3 though.)
          Hide
          Erick Erickson added a comment -

          The more the merrier. I've done most of the legwork in SOLR-4615 (removed the whole solr.properties thing, un-deprecated solr.xml, fixed up the tests, etc) so you'll probably want to either wait until I check that in (Real Soon Now) or work off that patch/JIRA.

          Show
          Erick Erickson added a comment - The more the merrier. I've done most of the legwork in SOLR-4615 (removed the whole solr.properties thing, un-deprecated solr.xml, fixed up the tests, etc) so you'll probably want to either wait until I check that in (Real Soon Now) or work off that patch/JIRA.
          Hide
          Erick Erickson added a comment -

          I messed up a little and didn't get the trunk patch (committed then tried to cut the final patch). So this patch is for 4x after merging from trunk.

          trunk r: 1463316

          I'm going to make sure this works with 4x, check it in and close this JIRA. Before I do that I'll open another blocker for straightening out solr.xml (however we're going to do that). At least this way people won't get be going down the solr.properties path and pulling the attribute out of <cores> shouldn't be hard.

          Show
          Erick Erickson added a comment - I messed up a little and didn't get the trunk patch (committed then tried to cut the final patch). So this patch is for 4x after merging from trunk. trunk r: 1463316 I'm going to make sure this works with 4x, check it in and close this JIRA. Before I do that I'll open another blocker for straightening out solr.xml (however we're going to do that). At least this way people won't get be going down the solr.properties path and pulling the attribute out of <cores> shouldn't be hard.
          Hide
          Erick Erickson added a comment -

          We need to track these two together.

          Show
          Erick Erickson added a comment - We need to track these two together.
          Hide
          Erick Erickson added a comment -

          4.x r: 1463342

          Track the rest of straightening out how we deal with solr.xml in SOLR-4662, also a blocker.

          Show
          Erick Erickson added a comment - 4.x r: 1463342 Track the rest of straightening out how we deal with solr.xml in SOLR-4662 , also a blocker.

            People

            • Assignee:
              Erick Erickson
              Reporter:
              Erick Erickson
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development