[GitHub] lucene-solr issue #528: SOLR-12955 2
Github user barrotsteindev commented on the issue: https://github.com/apache/lucene-solr/pull/528 > postProcessDeleteByQuery can be renamed to doDistribDeleteByQuery, and postProcessDeleteById can be renamed doDistribDeleteById. I pushed a commit with these requested changes. > Let me know when you're done with the issue and I can try some thing too. It's such a time sink unfortunately. I can not seem to come up with a way to deal with doDeleteById & doDeleteByQuery, and I do not have a lot of time to sit and think about this, so I will lay off this issue for a while. Feel free to have a go at it as well, perhaps you might find a neater solution. --- - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr issue #528: SOLR-12955 2
Github user dsmiley commented on the issue: https://github.com/apache/lucene-solr/pull/528 Man this is a tangled mess we are trying to unravel! I wish someone undertook this years ago. Lets not rename DUP#processDelete as it is an URP defined method we override, and I doubt it'd worth it to expand the change to URP. postProcessDeleteByQuery can be renamed to doDistribDeleteByQuery, and postProcessDeleteById can be renamed doDistribDeleteById. The flow of doDeleteById & doDeleteByQuery are different, which is a bit annoying. It's hard for me to make much sense of it right now. I know the ZK version entirely overrides processCommit but I think there the flow was more clear, at least to me. But on the delete side, I find it confusing that the ZK version overrides doDeleteByQuery. ... gotta go for now Let me know when you're done with the issue and I can try some thing too. It's such a time sink unfortunately. --- - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr issue #528: SOLR-12955 2
Github user barrotsteindev commented on the issue: https://github.com/apache/lucene-solr/pull/528 > It'd be excellent if we could get some consistency of calling setupRequest -- all processXXX methods should first call setupRequest and if needed an overloaded variant. Perhaps we'd have to rename DUP#processDelete, since it only routes the DeleteUpdateCommand to DUP#doDeleteById or DUP#doDeleteByQuery, the latter having a unique logic is is different to setupRequest. Another option would be to check what type of UpdateCommand is passed to DUP#setupRequest, but that would mean there might be some duplication of DeleteUpdateCommand checks in DistributedUpdateCommandProcessor and DistributedZkUPdateProcessor. WDYT? --- - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr issue #528: SOLR-12955 2
Github user dsmiley commented on the issue: https://github.com/apache/lucene-solr/pull/528 I like that the zk version of processCommit completely overrides the superclass since the logic is much more complicated. But it's a little unfortunate to call super.processCommit which will have some redundant setupRequest. One slight change would be for these method calls to change back to call localCommit() as they did before, but also put the cmd.setVersion logic at the front of localCommit's logic since this is always done first. --- - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr issue #528: SOLR-12955 2
Github user dsmiley commented on the issue: https://github.com/apache/lucene-solr/pull/528 * DUP.next can be removed as it's redundant with the same in the superclass * DUP.MAX_RETRIES_TO_FOLLOWERS_DEFAULT you accidentally added an 'R' to the ** of the javadoc. * DUP.isLeader method ought to go after the constructor. Generally, the constructor goes before instance methods. * DUP.isIndexChanged() remove this method; strictly internal and it's inconsistent with the fact that there are many fields that don't have accessors, so why should this one. * DUP.versionAdd shouldClone: I think it would be a little simpler to have the second conditional to "shouldClone" instead check if clonedDoc != null. At that point you will only have one var reference to shouldClone and you could inline it to how you initialize it. * DUP.postProcessAdd: I think this method would be better named doDistribAdd * Maybe we can avoid DistributedZkUpdateProcessor overriding processAdd. Somehow checkReplicationTracker() needs to get called. One way to do this would be for setupRequest to check if the updateCommand is an instance of AddUpdateCommand)). * It'd be excellent if we could get some consistency of calling setupRequest -- all processXXX methods should first call setupRequest and if needed an overloaded variant. --- - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr issue #528: SOLR-12955 2
Github user dsmiley commented on the issue: https://github.com/apache/lucene-solr/pull/528 * DUP.postProcessAdd's javadoc is confusing; maybe drop it. It says "This method can be overridden to tamper with the cmd" but I'm looking at where we override it and I don't see any tampering to this.updateCommand. * DUP: Lets remove the Standalone subclass in favor of making the base class do the standalone functionality; I think it's not really doing much of anything other than relying on the base impl. There's practically no functionality (real code) in the standalone class right now so it's kind of pointless. * DistributedZkUpdateProcessor's constructor that takes a docMerger isn't called externally so lets have just one constructor --- - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr issue #528: SOLR-12955 2
Github user barrotsteindev commented on the issue: https://github.com/apache/lucene-solr/pull/528 Oh yes, I still have a few Todo's I added that I'd like to tackle. I won't have time for more today, Hopefully I'll be able to get these done by the end of the week. --- - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr issue #528: SOLR-12955 2
Github user dsmiley commented on the issue: https://github.com/apache/lucene-solr/pull/528 Looks good. More to do? --- - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr issue #528: SOLR-12955 2
Github user barrotsteindev commented on the issue: https://github.com/apache/lucene-solr/pull/528 addressed comments about previous PR and re-based on latest master --- - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] lucene-solr issue #528: SOLR-12955 2
Github user barrotsteindev commented on the issue: https://github.com/apache/lucene-solr/pull/528 Tackling this again, according to @dsmiley's input: > I'm not sure if the "base" functionality needed to be in a new class... I could imagine simply adding subclasses and leave DUP named as it is with it's base functionality. as stated in pr [519](https://github.com/apache/lucene-solr/pull/519) --- - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
