Solr
  1. Solr
  2. SOLR-4910

solr.xml persistence is completely broken

    Details

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

      Description

      I'm working on SOLR-4862 (persisting a created core doesn't preserve some values) and at least compared to 4.3 code, persisting to solr.xml is completely broken.

      I learned to hate persistence while working on SOLR-4196 & etc. and I'm glad it's going away. I frequently got lost in implicit properties (they're easy to persist and shouldn't be), what should/shouldn't be persisted (e.g. the translated $

      {var:default}

      or the original), and it was a monster, so don't think I'm nostalgic for the historical behavior.

      Before I dive back in I want to get some idea whether or not the current behavior was intentional or not, I don't want to go back into that junk only to undo someone else's work.

      Creating a new core (collection2 in my example) with persistence turned on in solr.xml for instance changes the original definition for collection1 (stock 4.x as of tonight) from this:

      <core name="collection1" instanceDir="collection1" shard="$

      {shard:}" collection="${collection:collection1}" config="${solrconfig:solrconfig.xml}" schema="${schema:schema.xml}"
      coreNodeName="${coreNodeName:}"/>
      to this:


      <core loadOnStartup="true" shard="${shard}

      " instanceDir="collection1/" transient="false" name="collection1" dataDir="data/" collection="$

      {collection:collection1}

      ">
      <property name="name" value="collection1"/>
      <property name="config" value="solrconfig.xml"/>
      <property name="solr.core.instanceDir" value="solr/collection1/"/>
      <property name="transient" value="false"/>
      <property name="schema" value="schema.xml"/>
      <property name="loadOnStartup" value="true"/>
      <property name="solr.core.schemaName" value="schema.xml"/>
      <property name="solr.core.name" value="collection1"/>
      <property name="solr.core.dataDir" value="data/"/>
      <property name="instanceDir" value="collection1/"/>
      <property name="solr.core.configName" value="solrconfig.xml"/>
      </core>

      So, there are two questions:
      1> what is correct for 4.x?
      2> do we care at all about 5.x?

      As much as I hate to say it, I think that we need to go back to the 4.3 behavior. It might be as simple as not persisting in the <property> tags anything already in the original definition. Not quite sure what to put where in the newly-created core though, I suspect that the compact <core + attribs> would be best (assuming there's no <property> tag already in the definition. I really hate the mix of attributes on the <core> tag and <property> tags, wish we had one or the other....

      1. SOLR-4910.patch
        54 kB
        Erick Erickson
      2. SOLR-4910.patch
        44 kB
        Erick Erickson
      3. SOLR-4910.patch
        36 kB
        Erick Erickson
      4. SOLR-4910.patch
        16 kB
        Erick Erickson
      5. SOLR-4910.patch
        38 kB
        Alan Woodward

        Issue Links

          Activity

          Hide
          Erick Erickson added a comment -

          From the user's list (Mark Miller) since the JIRA was down for maintenance.

          *****
          I think in the past, it simply tried to write out what it read in. That should be the end goal, and AFAIK things pretty much worked like that in the late 3's or early 4's.

          I'm pretty sure for ${} sys prop notation, 4.3 broke the <cores section, but now it sounds like the <core part is broken. Just from memory though - I know something wasn't working in 4.3 along those lines. So the real issue seems to be the current testing...

          We have a some pretty simple tests for this (eg, they are easy to add too), so it sounds like we just need to beef them up - the changes necessary are easy from there.

          ******

          Show
          Erick Erickson added a comment - From the user's list (Mark Miller) since the JIRA was down for maintenance. ***** I think in the past, it simply tried to write out what it read in. That should be the end goal, and AFAIK things pretty much worked like that in the late 3's or early 4's. I'm pretty sure for ${} sys prop notation, 4.3 broke the <cores section, but now it sounds like the <core part is broken. Just from memory though - I know something wasn't working in 4.3 along those lines. So the real issue seems to be the current testing... We have a some pretty simple tests for this (eg, they are easy to add too), so it sounds like we just need to beef them up - the changes necessary are easy from there. ******
          Hide
          Erick Erickson added a comment -

          bq: So the real issue seems to be the current testing...

          Not quite sure what you mean here. I fully agree that current testing isn't catching this kind of problem, but the underlying behavior is broken too.

          bq: I'm pretty sure for ${} sys prop notation, 4.3 broke the <cores

          Absolutely possible.....

          Let me see what I can do, the hack patch has the triage code in the wrong place, it should go in SolrCores when we build the objects we pass in to be persisted, and I'll see what I can do as far as beefing up the tests. So here are the rules I'm thinking of.

          1> preserve the original if present. Since we have access to the original DOM object from solr.xml, this should be easy. Although the core name change/swap may be a problem here, what should be written out? There's a bunch of stuff about coreToOrigName in there.

          2> don't persist the implicit properties (things prefixed with solr.core) as one of the <property> tags.

          3> everything else as it is now, especially the <cores> bits, I'm thinking this just really applies to the <core> tags.

          4> add much beefier tests.

          I'll take a whack at this tomorrow morning and see how far I get.

          Show
          Erick Erickson added a comment - bq: So the real issue seems to be the current testing... Not quite sure what you mean here. I fully agree that current testing isn't catching this kind of problem, but the underlying behavior is broken too. bq: I'm pretty sure for ${} sys prop notation, 4.3 broke the <cores Absolutely possible..... Let me see what I can do, the hack patch has the triage code in the wrong place, it should go in SolrCores when we build the objects we pass in to be persisted, and I'll see what I can do as far as beefing up the tests. So here are the rules I'm thinking of. 1> preserve the original if present. Since we have access to the original DOM object from solr.xml, this should be easy. Although the core name change/swap may be a problem here, what should be written out? There's a bunch of stuff about coreToOrigName in there. 2> don't persist the implicit properties (things prefixed with solr.core) as one of the <property> tags. 3> everything else as it is now, especially the <cores> bits, I'm thinking this just really applies to the <core> tags. 4> add much beefier tests. I'll take a whack at this tomorrow morning and see how far I get.
          Hide
          Erick Erickson added a comment -

          So before I dive into this, I want to be sure I understand what the end result is, because core swapping and renaming isn't something I've dealt with a lot.

          corea is renamed coreb. Is coreb the persisted name?
          corea and coreb are swapped, what is persisted?

          Show
          Erick Erickson added a comment - So before I dive into this, I want to be sure I understand what the end result is, because core swapping and renaming isn't something I've dealt with a lot. corea is renamed coreb. Is coreb the persisted name? corea and coreb are swapped, what is persisted?
          Hide
          Shawn Heisey added a comment -

          corea is renamed coreb. Is coreb the persisted name?
          corea and coreb are swapped, what is persisted?

          I know a little bit about this, as I use core swapping in conjunction with full index rebuilds. Here's my opinion. Things that should be persisted any time there's a change:

          • Properties whose value has changed. In the case of a rename or swap, that would be the core name(s).
          • Properties that are explicitly stated in solr.xml or the core.properties that is being overwritten.
          • Properties whose default value will be different in a future version of Solr.

          Except as described in the list above, I don't think that unspecified defaults should be written. When I took my solr.xml from 3.5.0 and put it on 4.1-SNAPSHOT, I suddenly had new data in my solr.xml after a core swap, the settings for LotsOfCores. I don't think that should have been written, because it wasn't there in the first place.

          Show
          Shawn Heisey added a comment - corea is renamed coreb. Is coreb the persisted name? corea and coreb are swapped, what is persisted? I know a little bit about this, as I use core swapping in conjunction with full index rebuilds. Here's my opinion. Things that should be persisted any time there's a change: Properties whose value has changed. In the case of a rename or swap, that would be the core name(s). Properties that are explicitly stated in solr.xml or the core.properties that is being overwritten. Properties whose default value will be different in a future version of Solr. Except as described in the list above, I don't think that unspecified defaults should be written. When I took my solr.xml from 3.5.0 and put it on 4.1-SNAPSHOT, I suddenly had new data in my solr.xml after a core swap, the settings for LotsOfCores. I don't think that should have been written, because it wasn't there in the first place.
          Hide
          Alan Woodward added a comment -

          I'm glad it's going away

          Well, it isn't really, is it? The point of persistence is to make sure that changes made by the CoreAdminHandler (creations, deletions, renames, aliases and swaps, plus splits and merges which are special cases of create/delete) are still there after you restart. And this is the case both if you're writing everything into solr.xml or if you're updating a core.properties file.

          What would be really good here would be to split persistence policies out into separate classes, one for writing to solr.xml and one for writing core.properties. For solr.xml, the only mutable part is the cores list - we can store the surrounding part as a plain String, which means a) we don't need to worry about reconstructing things from a DOM, so we're guaranteed that we'll always write out what we read in, and b) we can preserve things like XML comments. The core.properties version just writes out any non-default properties, subject to the rules that Shawn outlined above.

          The persistor (CoreDescriptorPersistor maybe? Mmm, euphonious...) would belong to the CoreContainer, which can then call persistor.persist(CoreDescriptor) after each relevant operation. CoreAdminHandler doesn't need to care about it, and we remove persist and persistFile as public methods of CoreContainer which reduces it's surface area a bit.

          CoreContainer, CoreDescriptor, CoreAdminHandler and ConfigSolr are all a bit leaky at the moment. I think this is a good opportunity to patch up some of these abstractions.

          Show
          Alan Woodward added a comment - I'm glad it's going away Well, it isn't really, is it? The point of persistence is to make sure that changes made by the CoreAdminHandler (creations, deletions, renames, aliases and swaps, plus splits and merges which are special cases of create/delete) are still there after you restart. And this is the case both if you're writing everything into solr.xml or if you're updating a core.properties file. What would be really good here would be to split persistence policies out into separate classes, one for writing to solr.xml and one for writing core.properties. For solr.xml, the only mutable part is the cores list - we can store the surrounding part as a plain String, which means a) we don't need to worry about reconstructing things from a DOM, so we're guaranteed that we'll always write out what we read in, and b) we can preserve things like XML comments. The core.properties version just writes out any non-default properties, subject to the rules that Shawn outlined above. The persistor (CoreDescriptorPersistor maybe? Mmm, euphonious...) would belong to the CoreContainer, which can then call persistor.persist(CoreDescriptor) after each relevant operation. CoreAdminHandler doesn't need to care about it, and we remove persist and persistFile as public methods of CoreContainer which reduces it's surface area a bit. CoreContainer, CoreDescriptor, CoreAdminHandler and ConfigSolr are all a bit leaky at the moment. I think this is a good opportunity to patch up some of these abstractions.
          Hide
          Alan Woodward added a comment -

          Patch, untested, full of nocommits, etc, etc, but shows where I'm thinking we should go with this.

          Show
          Alan Woodward added a comment - Patch, untested, full of nocommits, etc, etc, but shows where I'm thinking we should go with this.
          Hide
          Erick Erickson added a comment -

          Alan:

          Interesting stuff, looks like it would be much more rational, but far more ambitious than I was thinking. Not to say it shouldn't be carried forward, but I think it should be a separate issue. And only on the 5.x branch?

          I'm thinking we need to split this JIRA into two parts, tactical and strategic. The solr.xml problem blocks 4.4 IMO, and we might be cutting a 4.4 fairly soon. This approach feels like it needs some time to bake given how many changes it makes.

          So I've opened up a new JIRA (SOLR-4914) and attached your (renamed) patch. I'll carry 4910 forward in the short term and we can iterate on 4914 ongoing for the longer term.

          Show
          Erick Erickson added a comment - Alan: Interesting stuff, looks like it would be much more rational, but far more ambitious than I was thinking. Not to say it shouldn't be carried forward, but I think it should be a separate issue. And only on the 5.x branch? I'm thinking we need to split this JIRA into two parts, tactical and strategic. The solr.xml problem blocks 4.4 IMO, and we might be cutting a 4.4 fairly soon. This approach feels like it needs some time to bake given how many changes it makes. So I've opened up a new JIRA ( SOLR-4914 ) and attached your (renamed) patch. I'll carry 4910 forward in the short term and we can iterate on 4914 ongoing for the longer term.
          Hide
          Mark Miller added a comment -

          Not quite sure what you mean here. I fully agree that current testing isn't catching this kind of problem, but the underlying behavior is broken too.

          I mean that I think both of these things have been broken and fixed recently - that means testing is hitting none of this, so a fix is worthless without tests to make sure this stuff works. Adding tests requires a fix to the underlying problem. So what I am saying is that the majority of thought and effort on this should not be "how do we fix it", and instead spent on adding tests and making them pass.

          It's very easy to check and see what the old behavior was regarding peristence - it was pretty simple though - we try and write out what we read in, but if the change was triggered dynamically, we can write it out however we want.

          It's a little odd, but TestSolrProperties is the current class that tests this type of stuff fairly simply. I think that is what we really want to beef up. I can likely pitch in as well.

          Show
          Mark Miller added a comment - Not quite sure what you mean here. I fully agree that current testing isn't catching this kind of problem, but the underlying behavior is broken too. I mean that I think both of these things have been broken and fixed recently - that means testing is hitting none of this, so a fix is worthless without tests to make sure this stuff works. Adding tests requires a fix to the underlying problem. So what I am saying is that the majority of thought and effort on this should not be "how do we fix it", and instead spent on adding tests and making them pass. It's very easy to check and see what the old behavior was regarding peristence - it was pretty simple though - we try and write out what we read in, but if the change was triggered dynamically, we can write it out however we want. It's a little odd, but TestSolrProperties is the current class that tests this type of stuff fairly simply. I think that is what we really want to beef up. I can likely pitch in as well.
          Hide
          Erick Erickson added a comment -

          OK, thanks for the clarification. I'll see what I can start getting together tonight and we can iterate...

          Show
          Erick Erickson added a comment - OK, thanks for the clarification. I'll see what I can start getting together tonight and we can iterate...
          Hide
          Alan Woodward added a comment -

          OK, I can see you want to hold off anything major if we're dropping 4.4 imminently, but I think it's really worth looking at rationalizing this stuff. Part of the reason it's easy to break, in addition to the lack of test coverage, is that the logic is spread out amongst a whole bunch of classes.

          Show
          Alan Woodward added a comment - OK, I can see you want to hold off anything major if we're dropping 4.4 imminently, but I think it's really worth looking at rationalizing this stuff. Part of the reason it's easy to break, in addition to the lack of test coverage, is that the logic is spread out amongst a whole bunch of classes.
          Hide
          Mark Miller added a comment -

          is that the logic is spread out amongst a whole bunch of classes.

          I agree - it's gotten quite difficult to deal with the persistence code.

          At one point, I started trying to isolate the code a bit - in an attempt to make testing easier - a fair bit was still left in CoreContainer still though. The new stuff moved away from simply keeping the old dom in memory (I think we still want to do that and actually reintroduced it at one point, though I didn't move all the code to use it) and spread the code across a variety of new classes.

          I started a large refactoring wave on it actually, but there remains much to do in general IMO. CoreContainer and all of this has started to grow unweildy. Things will simplify some even with no effort I think - just not having to support the two core discovery modes will make a better design easier.

          In any case, I'm not so happy with the current state of things, but currently two things have largely affected my thoughts on that:

          1. I have not had much time to apply to this problem - I started a lot of work a month or two back and have yet to come back to it.
          2. We are dropping all this persistence stuff shortly in 5, as well as support for the old style solr.xml.

          So while we have a pressing needs for tests and fixes around solr.xml persistence, anything else I consider just gravy that may slide off into the trash when we release 5. Who doesn't love gravy if someone is willing to serve it But I'd be almost as happy if we simply got this working as a start - then if people want to improve it, patches welcome, but we get to clean slate it in 5.0 anyway (when it comes to solr.xml persistence).

          I do welcome any contributions or improvements - but until we actually nail down the testing (this refactoring has exposed how few we have in these areas), refacoring is pretty scary stuff.

          Show
          Mark Miller added a comment - is that the logic is spread out amongst a whole bunch of classes. I agree - it's gotten quite difficult to deal with the persistence code. At one point, I started trying to isolate the code a bit - in an attempt to make testing easier - a fair bit was still left in CoreContainer still though. The new stuff moved away from simply keeping the old dom in memory (I think we still want to do that and actually reintroduced it at one point, though I didn't move all the code to use it) and spread the code across a variety of new classes. I started a large refactoring wave on it actually, but there remains much to do in general IMO. CoreContainer and all of this has started to grow unweildy. Things will simplify some even with no effort I think - just not having to support the two core discovery modes will make a better design easier. In any case, I'm not so happy with the current state of things, but currently two things have largely affected my thoughts on that: 1. I have not had much time to apply to this problem - I started a lot of work a month or two back and have yet to come back to it. 2. We are dropping all this persistence stuff shortly in 5, as well as support for the old style solr.xml. So while we have a pressing needs for tests and fixes around solr.xml persistence, anything else I consider just gravy that may slide off into the trash when we release 5. Who doesn't love gravy if someone is willing to serve it But I'd be almost as happy if we simply got this working as a start - then if people want to improve it, patches welcome, but we get to clean slate it in 5.0 anyway (when it comes to solr.xml persistence). I do welcome any contributions or improvements - but until we actually nail down the testing (this refactoring has exposed how few we have in these areas), refacoring is pretty scary stuff.
          Hide
          Erick Erickson added a comment -

          Here's the start of a patch that does nothing except
          1> read in an solr.xml file
          2> persist it
          3> fail because the written file is different

          Mind you, this is just barely past the point of working at all, so it's pretty ragged but I thought I'd put it up as a place to start. I hope to have a full patch ready by Monday...

          Show
          Erick Erickson added a comment - Here's the start of a patch that does nothing except 1> read in an solr.xml file 2> persist it 3> fail because the written file is different Mind you, this is just barely past the point of working at all, so it's pretty ragged but I thought I'd put it up as a place to start. I hope to have a full patch ready by Monday...
          Hide
          Erick Erickson added a comment -

          bq (Alan): OK, I can see you want to hold off anything major if we're dropping 4.4 imminently, but I think it's really worth looking at rationalizing this stuff

          I completely agree, it's pretty clear that this grew organically and is a hodgepodge at best. It's just that I'm pretty uncomfortable with the timing. Once 4.4 is cut, particularly if we can do something very quickly after 4.4 goes out and that discomfort goes away. Much of my trepidation is that there's some agitation for 4.4 in the very near future.

          bq (Mark): We are dropping all this persistence stuff shortly in 5, as well as support for the old style solr.xml.

          You don't know how thrilled I am to be working on something that I know is going to be thrown out <G>.. But I'm feeling like I'm earning the right to be the one who rips it out <GGG>.

          bq (Mark): But I'd be almost as happy if we simply got this working as a start

          Right, that's about all I intend to do on this. When 5.0 comes along and we'll rip it entirely out. So I'm looking at just enough to get by the current hurdles and declare victory.

          The bulk of the effort in what I did tonight was figuring out how to verify that all the nodes and attributes in one xml file are identical to another. Please don't tell me something's already been written, I looked but didn't find it. Of course what I made tonight is specialized to solr.xml and I suspect it has some holes to flush out as the patch matures.

          Show
          Erick Erickson added a comment - bq (Alan): OK, I can see you want to hold off anything major if we're dropping 4.4 imminently, but I think it's really worth looking at rationalizing this stuff I completely agree, it's pretty clear that this grew organically and is a hodgepodge at best. It's just that I'm pretty uncomfortable with the timing. Once 4.4 is cut, particularly if we can do something very quickly after 4.4 goes out and that discomfort goes away. Much of my trepidation is that there's some agitation for 4.4 in the very near future. bq (Mark): We are dropping all this persistence stuff shortly in 5, as well as support for the old style solr.xml. You don't know how thrilled I am to be working on something that I know is going to be thrown out <G>.. But I'm feeling like I'm earning the right to be the one who rips it out <GGG>. bq (Mark): But I'd be almost as happy if we simply got this working as a start Right, that's about all I intend to do on this. When 5.0 comes along and we'll rip it entirely out. So I'm looking at just enough to get by the current hurdles and declare victory. The bulk of the effort in what I did tonight was figuring out how to verify that all the nodes and attributes in one xml file are identical to another. Please don't tell me something's already been written, I looked but didn't find it. Of course what I made tonight is specialized to solr.xml and I suspect it has some holes to flush out as the patch matures.
          Hide
          Mark Miller added a comment -

          The bulk of the effort in what I did tonight was figuring out how to verify that all the nodes and attributes in one xml file are identical to another.

          Nice! That will be useful.

          Show
          Mark Miller added a comment - The bulk of the effort in what I did tonight was figuring out how to verify that all the nodes and attributes in one xml file are identical to another. Nice! That will be useful.
          Hide
          Erick Erickson added a comment -

          bq: Nice! That will be useful.

          It's rather specialized to solr.xml, but perhaps it can be generalized. The idea is that it creates XPATH expressions so equality is expressed as
          are all xpath expressions created from xml A in xml B and vice-versa

          Show
          Erick Erickson added a comment - bq: Nice! That will be useful. It's rather specialized to solr.xml, but perhaps it can be generalized. The idea is that it creates XPATH expressions so equality is expressed as are all xpath expressions created from xml A in xml B and vice-versa
          Hide
          Erick Erickson added a comment -

          Latest patch. I threw together a basket of lots and lots of things that could go in a solr.xml file. Along the way I found out that we weren't persisting a bunch of things, so I added them. It's possible I got confused about what's valid in 4.x as opposed to 5.x, so any other eyes would be welcome.

          The current state is that we can successfully persist about a zillion things in the solr.xml file. I still have to write some more tests, particularly around creating a new core and some more attempts to use system variables and insure that the original ${} syntax is preserved. Also, renaming cores and insuring that persistence is correct.

          I've also grabbed some other JIRAs that have to do with persisting solr.xml, please bring to my attention any that aren't already assigned to me.

          Show
          Erick Erickson added a comment - Latest patch. I threw together a basket of lots and lots of things that could go in a solr.xml file. Along the way I found out that we weren't persisting a bunch of things, so I added them. It's possible I got confused about what's valid in 4.x as opposed to 5.x, so any other eyes would be welcome. The current state is that we can successfully persist about a zillion things in the solr.xml file. I still have to write some more tests, particularly around creating a new core and some more attempts to use system variables and insure that the original ${} syntax is preserved. Also, renaming cores and insuring that persistence is correct. I've also grabbed some other JIRAs that have to do with persisting solr.xml, please bring to my attention any that aren't already assigned to me.
          Hide
          Mark Miller added a comment -

          I don't think numShards should be persisted or be part of solr.xml currently - did you find somewhere it was? Because it can change, we have tried to keep it out of solr.xml.

          Show
          Mark Miller added a comment - I don't think numShards should be persisted or be part of solr.xml currently - did you find somewhere it was? Because it can change, we have tried to keep it out of solr.xml.
          Hide
          Erick Erickson added a comment -

          bq: I don't think numShards should be persisted or be part of solr.xml currently - did you find somewhere it was?

          Ahh, thanks. I saw it in: ...trunk/solr/core/src/test-files/solr/solr.xml and inferred it was a Good Thing (and it surprised me that it was). It looks like that snuck in there incorrectly, I'll remove it totally.

          Show
          Erick Erickson added a comment - bq: I don't think numShards should be persisted or be part of solr.xml currently - did you find somewhere it was? Ahh, thanks. I saw it in: ...trunk/solr/core/src/test-files/solr/solr.xml and inferred it was a Good Thing (and it surprised me that it was). It looks like that snuck in there incorrectly, I'll remove it totally.
          Hide
          Mark Miller added a comment -

          Yeah, that was probably me.

          Show
          Mark Miller added a comment - Yeah, that was probably me.
          Hide
          Erick Erickson added a comment -

          Getting much closer, I've got a couple of test failures to investigate, but so far most of the old tests that fail have been verifying bad behavior....

          I need to add tests for RELOAD, UNLOAD/LOAD, RENAME and SWAP. These may well uncover other stuff of course.

          There are a couple of nocommits still in, I just saw a really neat construct for when we mess with system vars that I'm trying, so I commented out where I was doing this manually, does this really reset all system vars?

          @Rule
          public TestRule solrTestRules =
          RuleChain.outerRule(new SystemPropertiesRestoreRule());

          All eyes welcome of course.

          Show
          Erick Erickson added a comment - Getting much closer, I've got a couple of test failures to investigate, but so far most of the old tests that fail have been verifying bad behavior.... I need to add tests for RELOAD, UNLOAD/LOAD, RENAME and SWAP. These may well uncover other stuff of course. There are a couple of nocommits still in, I just saw a really neat construct for when we mess with system vars that I'm trying, so I commented out where I was doing this manually, does this really reset all system vars? @Rule public TestRule solrTestRules = RuleChain.outerRule(new SystemPropertiesRestoreRule()); All eyes welcome of course.
          Hide
          Erick Erickson added a comment -

          OK, if all the tests pass (running now), I think this is ready and I'll put it up tonight or tomorrow unless there are objections.

          This patch goes against trunk...

          This takes care of 4 bugs and 5 things that testing flushed out, see solr/CHANGES.txt.

          Shawn (or anyone for that matter) if you have a chance to run this through any exercises it would be a Good Thing, especially seeing whether I made the right decisions around swapping and renaming.

          Under any circumstances, though, unless someone finds something horribly wrong or the tests blow up, I think this improves solr.xml persistence significantly and I'll check it in Real Soon Now.

          Show
          Erick Erickson added a comment - OK, if all the tests pass (running now), I think this is ready and I'll put it up tonight or tomorrow unless there are objections. This patch goes against trunk... This takes care of 4 bugs and 5 things that testing flushed out, see solr/CHANGES.txt. Shawn (or anyone for that matter) if you have a chance to run this through any exercises it would be a Good Thing, especially seeing whether I made the right decisions around swapping and renaming. Under any circumstances, though, unless someone finds something horribly wrong or the tests blow up, I think this improves solr.xml persistence significantly and I'll check it in Real Soon Now.
          Hide
          Shawn Heisey added a comment -

          That was a bit of a bear to apply to 4x, but I got it done. I don't have anything real set up with trunk where I can easily work on it with my index building code. Perhaps I should set up a fourth index chain for that.

          I will poke around a bit.

          Show
          Shawn Heisey added a comment - That was a bit of a bear to apply to 4x, but I got it done. I don't have anything real set up with trunk where I can easily work on it with my index building code. Perhaps I should set up a fourth index chain for that. I will poke around a bit.
          Hide
          Commit Tag Bot added a comment -

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

          SOLR-4910, improvements to persisting solr.xml and misc other fixes, see CHANGES.txt

          Show
          Commit Tag Bot added a comment - [trunk commit] erick http://svn.apache.org/viewvc?view=revision&revision=1493618 SOLR-4910 , improvements to persisting solr.xml and misc other fixes, see CHANGES.txt
          Hide
          Commit Tag Bot added a comment -

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

          SOLR-4910, improvements to persisting solr.xml and misc other fixes, see CHANGES.txt

          Show
          Commit Tag Bot added a comment - [branch_4x commit] erick http://svn.apache.org/viewvc?view=revision&revision=1493620 SOLR-4910 , improvements to persisting solr.xml and misc other fixes, see CHANGES.txt
          Hide
          Erick Erickson added a comment - - edited

          trunk: 1493618
          4x: 1493620

          Also fixed in this commit:
          SOLR-4862, CREATE fails to persist schema, config, and dataDir
          SOLR-4363, not persisting coreLoadThreads in <solr> tag
          SOLR-3900, logWatcher properties not persisted
          SOLR-4850, cores defined as loadOnStartup=true, transient=false can't be searched

          Shawn Heisey Have at it, let's open up new JIRAs for anything you find, probably just assign them to me.

          Alan Woodward This commit probably makes your life more difficult, you may want to do an update sooner rather than later, this got somewhat bigger than I expected.

          Show
          Erick Erickson added a comment - - edited trunk: 1493618 4x: 1493620 Also fixed in this commit: SOLR-4862 , CREATE fails to persist schema, config, and dataDir SOLR-4363 , not persisting coreLoadThreads in <solr> tag SOLR-3900 , logWatcher properties not persisted SOLR-4850 , cores defined as loadOnStartup=true, transient=false can't be searched Shawn Heisey Have at it, let's open up new JIRAs for anything you find, probably just assign them to me. Alan Woodward This commit probably makes your life more difficult, you may want to do an update sooner rather than later, this got somewhat bigger than I expected.
          Hide
          Commit Tag Bot added a comment -

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

          SOLR-4910, corrected typo in CHANGES.txt

          Show
          Commit Tag Bot added a comment - [branch_4x commit] erick http://svn.apache.org/viewvc?view=revision&revision=1493621 SOLR-4910 , corrected typo in CHANGES.txt
          Hide
          Commit Tag Bot added a comment -

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

          SOLR-4910, corrected typo in CHANGES.txt

          Show
          Commit Tag Bot added a comment - [trunk commit] erick http://svn.apache.org/viewvc?view=revision&revision=1493622 SOLR-4910 , corrected typo in CHANGES.txt
          Hide
          Shawn Heisey added a comment - - edited

          After stripping my solr.xml down to the minimum required, I did a rebuild/swap. It looks like the only thing that got added was these attributes to the cores tag:

          distribUpdateSoTimeout="0" distribUpdateConnTimeout="0"

          If these are parameters whose value will likely have a different default in a future Solr version, then this meets the criteria I outlined. If not, then IMHO they shouldn't have been put in there. That said, this is a whole lot better than it was before. I had already upgraded this test machine to branch_4x and run it through swaps before. The solr.xml that I edited before this test run was HUGE.

          I did leave a couple of extraneous bits (transient and collection) around, those were still there after the swap, so that's awesome.

          This wasn't an exhaustive test. I haven't looked at the tests added in your patch, but if those are pretty complete, then I don't think I need to look deeper.

          Show
          Shawn Heisey added a comment - - edited After stripping my solr.xml down to the minimum required, I did a rebuild/swap. It looks like the only thing that got added was these attributes to the cores tag: distribUpdateSoTimeout="0" distribUpdateConnTimeout="0" If these are parameters whose value will likely have a different default in a future Solr version, then this meets the criteria I outlined. If not, then IMHO they shouldn't have been put in there. That said, this is a whole lot better than it was before. I had already upgraded this test machine to branch_4x and run it through swaps before. The solr.xml that I edited before this test run was HUGE. I did leave a couple of extraneous bits (transient and collection) around, those were still there after the swap, so that's awesome. This wasn't an exhaustive test. I haven't looked at the tests added in your patch, but if those are pretty complete, then I don't think I need to look deeper.
          Hide
          Mark Miller added a comment -

          After stripping my solr.xml down to the minimum required, I did a rebuild/swap. It looks like the only thing that got added was these attributes to the cores tag:

          Good test and nice catch!

          Show
          Mark Miller added a comment - After stripping my solr.xml down to the minimum required, I did a rebuild/swap. It looks like the only thing that got added was these attributes to the cores tag: Good test and nice catch!
          Hide
          Erick Erickson added a comment -

          Shawn Heisey Thanks! I'll open up a new JIRA, this should be easy to fix. I think the tests didn't catch that b/c they were in the <cores> tag already, so this should be a pretty easy thing to add a test for.

          Alan Woodward So there'll be one more bit to make your life a little more difficult, but this should be a very small change.

          Show
          Erick Erickson added a comment - Shawn Heisey Thanks! I'll open up a new JIRA, this should be easy to fix. I think the tests didn't catch that b/c they were in the <cores> tag already, so this should be a pretty easy thing to add a test for. Alan Woodward So there'll be one more bit to make your life a little more difficult, but this should be a very small change.
          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:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development