[GitHub] zookeeper issue #522: ZOOKEEPER-1919 Update the C implementation of removeWa...

2018-05-29 Thread phunt
Github user phunt commented on the issue:

https://github.com/apache/zookeeper/pull/522
  
Hm. I had some trouble testing this. In particular I ran into the 
following. Some of this might be due to the fact that I run ubuntu in a docker 
container on my mac, but I think it could be an issue regardless:

1) tests/zkServer.sh isn't sleeping long enough and the client gives up too 
soon (RO mode takes a while to kick in)
2) IPV6 is identified but failing
3) remove watches wasn't added to the c cli so you can't exercise outside 
the test.

I'll enter jiras for these. Some are good Newbie issues.


---


[GitHub] zookeeper issue #522: ZOOKEEPER-1919 Update the C implementation of removeWa...

2018-05-28 Thread anmolnar
Github user anmolnar commented on the issue:

https://github.com/apache/zookeeper/pull/522
  
@phunt You might want to commit this as @rgs1 already reviewed it and you 
were also involved.


---


[GitHub] zookeeper issue #522: ZOOKEEPER-1919 Update the C implementation of removeWa...

2018-05-25 Thread anmolnar
Github user anmolnar commented on the issue:

https://github.com/apache/zookeeper/pull/522
  
@rgs1 Thanks for the review.
Would you please merge it?
Target branches are `master` and `3.5`


---


[GitHub] zookeeper issue #522: ZOOKEEPER-1919 Update the C implementation of removeWa...

2018-05-24 Thread rgs1
Github user rgs1 commented on the issue:

https://github.com/apache/zookeeper/pull/522
  
lgtm, r+. thanks!


---


[GitHub] zookeeper issue #522: ZOOKEEPER-1919 Update the C implementation of removeWa...

2018-05-22 Thread anmolnar
Github user anmolnar commented on the issue:

https://github.com/apache/zookeeper/pull/522
  
@rgs1 I think I got them all. Please validate.


---


[GitHub] zookeeper issue #522: ZOOKEEPER-1919 Update the C implementation of removeWa...

2018-05-16 Thread anmolnar
Github user anmolnar commented on the issue:

https://github.com/apache/zookeeper/pull/522
  
@rgs1 You might want to take a look at this patch and merge it.


---