[GitHub] lucene-solr issue #528: SOLR-12955 2

2019-01-06 Thread barrotsteindev
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

2018-12-31 Thread dsmiley
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

2018-12-30 Thread barrotsteindev
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

2018-12-26 Thread dsmiley
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

2018-12-26 Thread dsmiley
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

2018-12-26 Thread dsmiley
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

2018-12-17 Thread barrotsteindev
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

2018-12-17 Thread dsmiley
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

2018-12-17 Thread barrotsteindev
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

2018-12-15 Thread barrotsteindev
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