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]
