Well, I can yank the solr.properties stuff quite quickly and put in a new <cores> attribute autoDiscovery=true|false, which seems to be where we're headed. Off the top of my head, SolrProperties would morph into something like AutoCoreDiscovery and be called if the autoDiscovery=true property were found in solr.xml. From there it's just yanking the detection of solr.xml and renaming the SolrBackCompat class so it stays around, un-deprecating it, etc. There would be some cleanup, but not much. See: https://issues.apache.org/jira/browse/SOLR-4615
bq: I think it deserves a second pair of eyes Three or four. I wound up changing waaay more code than I'd originally thought I'd need to, you're not the only one with some discomfort about how big this turned out to be.... I think we should move the rest of this over to the JIRA so we have a better record of what happened. On Tue, Mar 19, 2013 at 8:54 AM, Mark Miller <[email protected]> wrote: > > On Mar 19, 2013, at 11:40 AM, Erick Erickson <[email protected]> > wrote: > > > bq: In particular, there is some code that says, don't lock for these it > causes deadlock > > > > There's no place where I intentionally put in code that didn't lock > shared variables etc. to avoid deadlock and then crossed my fingers hoping > all went well. Which says nothing about whether there are inadvertent > places of course. > > > > There are some notes about why locks are obtained long enough to copy > from shared variables to local variables since locking the shared stuff > while, say, closing cores would (and did) lead to deadlock, e.g. > CoreContainer.clearMaps(). That's a case where the rest of the locking is > supposed to suffice. > > > > And I remember one spot, although I can't find it right now, where I > intentionally did not obtain a lock but the code is, as I remember, > inaccessible except from code that _does_ have a lock. > > > > So point me at the code you're uncomfortable with and I'll be happy to > look it over again, it's possible that my cryptic comments which were > mostly to myself are misleading you. Of course it's also quite possible > that you're totally right and I screwed up, wouldn't be the first time….. > > I haven't had a chance to do a real review, so I'll wait to point out > anything specifically - my main point is that I think the code could use > some review and I think to this point it has not had any, at least that > I've picked up. It's been on my list a long time and I still hope to get to > it, but the fact that no one else has really looked yet, it makes me less > comfortable rushing such a large change out. I agree that the stress test > is comforting, but it still has some random fails - perhaps just test > fails, but it all just calls out for some more eyes because it's a rather > large, central change. This is a central part of Solr that everyone uses. > > It's not that I don't trust you, I just know how a big a change this is > and I think it deserves a second pair of eyes at least in some spots. I'm > mostly trying to frame the future when I talk about 5x - something like > this seems like it should have been in 5x prominently for a while. At this > point, it may be more work to go backward than forward on that front. I > think about this because I've been into the idea of releasing fairly often > on the 4.x branch - and Robert is a big releaser as well - so I'm going to > be paying close attention to issues that make it a little harder to just > release at any time. > > > > > I was kind of sad to see the pluggable core descriptor go, it seemed > like kind of a neat thing. But it didn't really have a compelling use-case > over auto-discovery so there's no good reason to bring it back. I suppose > if we bring it back (not suggesting it now, mind you) that we could use the > extracted manipulation in ConfigSolrXmlBackCompat (which will be renamed if > we pull the solr.properties file) as the template for an interface, but > that's for later. > > > > But the properties way of doing things did seem awkward, so I'm not > against yanking it. Much of the other code is there (I'm thinking about all > of the pending core operations) to address shortcomings that have been > there for a while. We've been able to lazily load/unload cores since 4.1, I > believe the stress test running against 4.1 would _not_ be pretty so taking > all that out seems like a mistake. > > If we can come to consensus on the next move, I'm happy to help dig into > some of this. I'm still hopeful that it might be a somewhat minor change > since it's really just altering the on disk format of the config file? > > - Mark > > > > > > > > > > > > On Tue, Mar 19, 2013 at 8:02 AM, Mark Miller <[email protected]> > wrote: > > > > On Mar 19, 2013, at 9:44 AM, Erick Erickson <[email protected]> > wrote: > > > > > So are you talking about backing all this out of 4x or just taking the > properties bits out? Because backing this all out of the 4x code line will > be...er...challenging, much more challenging than just yanking the > properties file bits out, this latter is much easier whereas reverting will > be a pain, I'll need some help there. I'm not really willing to re-do all > of the bits around preventing loading/unloading core operations occurring > at the same time from the ground up, it was too painful to do the first > time. But this isn't really a problem I don't think, since that code is > only relevant if you specify the lazy core loading options in your cores. > > > > I'm bringing up both things. The fact that neither our tests or example > use this new format yet scares me. It really feels like this didn't bake in > 5x at all if it wasn't part of the example or driving tests. The only > baking it had was it's own unit tests, but what it really needed was > dev/user face time. > > > > I'm open to driving this out in 4.3, but I'm pointing out where things > are at and that people should weigh in so we can solidify this and actually > get the changes into developers and users hands (beyond just the > refactoring that has gone into the old style back compat) or pull it from > 4x until it gets some real user time in 5x. > > > > > > > > I don't quite get what you mean by solr.xml and solrconfig.xml being > similar, just both being xml files? > > > > I think they should both be xml files and with similar style, yes. Or > eventually both changed to another format, but in my mind they should be > consistent. > > > > > And are we talking a discovery/no discovery attribute in, say, the > <cores> tag? > > > > Something like that - Robert tossed out some ideas, but we would have to > work through what is best. > > > > > Nor do I quite get the bits around threading. The stress code does > test the threading I think. > > > > There are shared variables that are accessed without locks - at least > there were a couple days ago when I took a casual look. That's a no no. > Might have to do with some of the random stress tests fails we still have, > but not get tickled by the stress tests - it's a bug in either case, so I > won't be happy with it until that code has gotten some more review. > > > > In particular, there is some code that says, don't lock for these it > causes deadlock - that's the biggest one I want to address - we can't > sacrifice safety in accessing shared vars to avoid deadlock - we have to > solve both. Now I have not looked in a few days, perhaps things have > shifted, but I'd like that code to get a solid review by someone else that > knows CoreContainer if possible. > > > > > It also specifically does both properties and xml versions of > solr.###, so it is part of our normal testing. Whether we can harden it > enough for 4.3 is certainly a legitimate question. > > > > This is my worry. I know I've discussed a 4.3 with Robert within a > relatively short time. Meanwhile, this large change that we will be locked > into with back compat is still not part of the example or driving our > tests. I'm worried it should have been put in 5x first, and made part of > the example at least. I realize it may be too later for that now, which is > why I'm straddling this middle path of, I don't know if this is ready for > 4.3 so let's either pull it or try and make it ready. I don't think you did > anything wrong to get us here, I'm just stating what I feel given where we > are. > > > > The real key to making it ready in my mind is to change the example and > force more people into facing the new setup. > > > > > > > > So are we reconsidering what I had a couple of months ago with > pluggable core discovery? The default would be to do things as they're done > now, we'd provide an implementation that walked the directory structure, > and dropping in a different version that, say, queried ZK or a database > would be pretty straight-forward. > > > > > > but if the solr.properties thing is to be killed, now is the time. > > > > I think we are only talking about the format of the core container > configuration. I don't think it really changes much of what you have done > or wanted to do - it's simple the format of the prop file. > > > > And getting more people to use that path so that we know it's right one > and shake out more problems/bugs! > > > > I'm less concerned with the stability (although that concerns me since > the new "way" is mostly hidden right now and so not getting hit by devs or > trunk users unless they really go out of the way) and more concerned around > being locked into what we do here. So I'm hoping some other committers are > forced to look closer at this before we release it. > > > > - Mark > > > > > > > > > > > > > > > > > On Mon, Mar 18, 2013 at 8:42 PM, Mark Miller <[email protected]> > wrote: > > > I've been thinking about this in the background. I tossed out some of > the idea's that were implemented here, and I also thought a properties file > rather than xml might make the most sense for container config. After all, > 99% of the things you set will be simple key->value props that are > attributes of the cores tag. I thought, wouldn't that just be simple as a > properties file? Easy hand wavy thought at the time. > > > > > > I had forgotten that the shard handler configuration had crept into > solr.xml. That made me think, what else will come creeping this way? Other > solrconfig.xml stuff that doesn't belong per SolrCore. If my memory is > right, shard handler was in solrconfig.xml at one point - so was the config > for zookeeper in the very first integration attempt with Solr. > > > > > > Shouldn't the Container and SolrCore config mostly match? Wouldn't it > be funny if was one yaml and the other xml? One a flat structure and the > other nested? I think so. > > > > > > So I'm changing my mind on the properties files and favoring the use > of the current solr.xml. It's a really easy switch for current users, it's > consistent with what we have, and we can get 99% of the benefits of Erick's > work even retaining solr.xml. > > > > > > I also think solr.xml and solrconfig.xml should be as similar as they > can be. > > > > > > The real problem with solr.xml is still solved with Erick's work - the > fact that cores are define there and Solr has to try and update the config > file. The file is really not so bad once we remove that little ugly wart. > > > > > > I think there would be a few details to work out, but this path is > very appealing to me. > > > > > > I don't think we should hurry this stuff - there is still some thread > safety concerns I have, and I'm not sure it's gotten a lot of use since > it's not part of the example or general tests. This is very important stuff > IMO, and we want to get this change right, even if that means it doesn't > make 4.3. 5x was almost invented for baking this type of change :) > > > > > > The faster we can come to a consensus, the faster we can get started > baking things though. > > > > > > Anyone voting for the properties file? > > > > > > - Mark > > > > > > On Mar 14, 2013, at 3:46 PM, Robert Muir <[email protected]> wrote: > > > > > > > I'm late to the game here, so I apologize if a lot of this has > already > > > > been discussed... > > > > > > > > I was looking recently and a little confused about the new > .properties > > > > format, a few questions: > > > > * is the current plan to deprecate the solr.xml support in 4.x and > drop for 5.0? > > > > * is there a real advantage to the .properties format over the > > > > existing .xml? When debugging it seemed I was in unknown territory a > > > > little bit, and this sorta means going forward that everything in > here > > > > is assumed to be flat: but it isnt really today, and what if more > > > > stuff needed to be added in the future that wasnt flat. For example > > > > the shard handler stuff has nested elements, but i kinda had to guess > > > > at how this mapped to .properties (since xml differentiates between > > > > attributes and elements, but its not so clear with .properties). > > > > > > > > It seems to me there are two changes involved: > > > > 1. ability to auto-discover cores from the filesystem so you don't > > > > need to explicitly list them > > > > 2. changing .xml format to .properties > > > > > > > > I guess just brainstorming, like what if we just kept the existing > > > > .xml format? we could add a new "autoDiscover=true" attribute to the > > > > cores element for people who don't want to list them explicitly. > > > > Additionally we could make the file optional (in case you have no > need > > > > to configure anything in it). This might be less of an intrusive > > > > change for 4.x too, as its a less radical change, just a new > attribute > > > > users can test if they want. Maybe we make it the default later in > 5.x > > > > but in general the format would still be the same, and we wouldnt > need > > > > the 2 different config implementations. > > > > > > > > --------------------------------------------------------------------- > > > > To unsubscribe, e-mail: [email protected] > > > > For additional commands, e-mail: [email protected] > > > > > > > > > > > > > --------------------------------------------------------------------- > > > To unsubscribe, e-mail: [email protected] > > > For additional commands, e-mail: [email protected] > > > > > > > > > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: [email protected] > > For additional commands, e-mail: [email protected] > > > > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > >
