Add an optional 'ZKMapListener' to ZooKeeperMap which fires as nodes are added, changed, or removed. #11

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants

cscotta commented Jan 24, 2012

Hi John,

We talked a bit about the contribution process for proposing and implementing enhancements for the "commons" lib a few weeks ago. Thanks for your help with the pants build process.

I've attached a diff that adds an optional "ZKMapListener" to ZooKeeperMap which allows users to be notified as child ZNodes are added, modified, and removed. The change preserves existing compatibility and interfaces, with the addition of a new constructor which optionally accepts the change listener. When nodes are added or their data changed, it fires "mapListener.nodeChanged(key, value)." Similarly when removed, the listener fires "mapListener.nodeRemoved(key)."

We're in the process of phasing in a stripped-down and mavenized version of the ZooKeeperClient and ZooKeeperMap libs with this modification to power our cluster membership and distributed stream processing at Boundary. We've been using this in production for about a week and the client seems to be working well -- thanks for releasing it (previously, we'd used Twitter's scala-zookeeper-client but noticed some odd behavior). I've begun the process of integrating this version of the client into Ordasity, our clustering library for stateful stream processing on the JVM (http://github.com/boundary/ordasity). The change to ZooKeeperMap allows Ordasity to be notified as topology and workload in ZooKeeper change, triggering it to reconfigure itself appropriately.

Let me know if you'd be interested in merging this pull request or if you have questions / concerns about the implementation, code style, or comments.

Thanks,

– Scott

C. Scott Andreas Add an optional 'ZKMapListener' to ZooKeeperMap which fires as nodes …
…are added, changed, and removed.
9639406

s/public // throughout - this is redundant

single blank line between body docs and @Param block

maybe just Listener? You already get namespacing via ZooKeeperMap.Listener

@nullable - or prefer a noop listener impl and remove the conditional logic - yay!

Tests? tests/java/com/twitter/common/zookeeper/ZooKeeperMapTest.java exists so it should be easy to add some listener tests.

Contributor

jsirois commented Jan 24, 2012

On Tue, Jan 24, 2012 at 12:42 PM, C. Scott Andreas <
reply@reply.github.com

wrote:

Hi John,

We talked a bit about the contribution process for proposing and
implementing enhancements for the "commons" lib a few weeks ago. Thanks for
your help with the pants build process.

I've attached a diff that adds an optional "ZKMapListener" to ZooKeeperMap
which allows users to be notified as child ZNodes are added, modified, and
removed. The change preserves existing compatibility and interfaces, with
the addition of a new constructor which optionally accepts the change
listener. When nodes are added or their data changed, it fires
"mapListener.nodeChanged(key, value)." Similarly when removed, the listener
fires "mapListener.nodeRemoved(key)."

We're in the process of phasing in a stripped-down and mavenized version
of the ZooKeeperClient and ZooKeeperMap libs with this modification to
power our cluster membership and

I'm really-really interested in this - pants makes slicing things up
incredibly easy. The current src/java/com/twitter/common/zookeeper target
can easily be broken into 1 per major api, ie:
src/java/com/twitter/common/zookeeper:client
src/java/com/twitter/common/zookeeper:map
src/java/com/twitter/common/zookeeper:group
...
etc.
Its several lines of edit to that file and some pushes.

distributed stream processing at Boundary. We've been using this in

production for about a week and the client seems to be working well --
thanks for releasing it (previously, we'd used Twitter's
scala-zookeeper-client but noticed some odd behavior). I've begun the
process of integrating this version of the client into Ordasity, our
clustering library for stateful stream processing on the JVM (
http://github.com/boundary/ordasity). The change to ZooKeeperMap allows
Ordasity to be notified as topology and workload in ZooKeeper change,
triggering it to reconfigure itself appropriately.

Let me know if you'd be interested in merging this pull request or if you
have questions / concerns about the implementation, code style, or comments.

Definitely interested - I commented on your diff.

Thanks,

Scott

You can merge this Pull Request by running:

git pull https://github.com/cscotta/commons master

Or you can view, comment on it, or merge it online at:

#11

-- Commit Summary --

  • Add an optional 'ZKMapListener' to ZooKeeperMap which fires as nodes are
    added, changed, and removed.

-- File Changes --

M src/java/com/twitter/common/zookeeper/ZooKeeperMap.java (83)

-- Patch Links --

https://github.com/twitter/commons/pull/11.patch
https://github.com/twitter/commons/pull/11.diff


Reply to this email directly or view it on GitHub:
#11

cscotta commented Jan 24, 2012

Thanks, John - sounds great. Really appreciate you checking this out quickly, and excellent suggestions above (esp. like the no-op listener idea). Should be able to have all of these wrapped up for you tonight.

– Scott

Contributor

jsirois commented Jan 24, 2012

If you have time, please check out this pull request: #12
It brings us from 1 fat jar (+1 testing jar):
(2.6.6) jsirois@tw-mbp17-jsirois ~/dev-twitter-commons (git::master) $ ./pants list src/java/com/twitter/common/zookeeper --provides
src/java/com/twitter/common/zookeeper/BUILD:zookeeper com.twitter.common#zookeeper
src/java/com/twitter/common/zookeeper/BUILD:testing com.twitter.common#zookeeper-testing

To 9 targeted jars:
(2.6.6) jsirois@tw-mbp17-jsirois ~/dev-twitter-commons (git::master) $ git co jsirois/zk/slice
Switched to branch 'jsirois/zk/slice'
Your branch is ahead of 'origin/master' by 1 commit.
(2.6.6) jsirois@tw-mbp17-jsirois ~/dev-twitter-commons (git::jsirois/zk/slice) $ ./pants list src/java/com/twitter/common/zookeeper --provides
src/java/com/twitter/common/zookeeper/BUILD:client com.twitter.common.zookeeper#client
src/java/com/twitter/common/zookeeper/BUILD:node com.twitter.common.zookeeper#node
src/java/com/twitter/common/zookeeper/BUILD:map com.twitter.common.zookeeper#map
src/java/com/twitter/common/zookeeper/BUILD:lock com.twitter.common.zookeeper#lock
src/java/com/twitter/common/zookeeper/BUILD:group com.twitter.common.zookeeper#group
src/java/com/twitter/common/zookeeper/BUILD:partitioner com.twitter.common.zookeeper#partitioner
src/java/com/twitter/common/zookeeper/BUILD:candidate com.twitter.common.zookeeper#candidate
src/java/com/twitter/common/zookeeper/BUILD:server-set com.twitter.common.zookeeper#server-set
src/java/com/twitter/common/zookeeper/BUILD:singleton-service com.twitter.common.zookeeper#singleton-service
src/java/com/twitter/common/zookeeper/BUILD:testing com.twitter.common#zookeeper-testing

If this looks good to you, I'll merge into master, merge back to twitter and push the jars.

Contributor

jsirois commented Jan 26, 2012

I'm going to merge in pull request 12 by EOD if I don't hear back. Will loop back on any feedback if so.

cscotta commented Jan 26, 2012

Thanks, John -- I'm sorry that I haven't had the chance to wrap my end of #11 up yet. PR#12 seems to make sense. I assume the purpose is to add the ability to build the ZooKeeper package separately from the rest of the lib? If so, fantastic.

Will try to wrap up my work this evening.

– Scott

Contributor

jsirois commented Jan 27, 2012

No worries - and yes, sort-of. The previous single zookeeper lib: http://maven.twttr.com/com/twitter/common/zookeeper/0.0.35/ is now broken into 9 libs - see dirs under http://maven.twttr.com/com/twitter/common/zookeeper/

$ for t in `pants list src/java/com/twitter/common/zookeeper`; do echo && pants depmap -im $t | grep zookeeper; done

com.twitter.common.zookeeper-client

com.twitter.common.zookeeper-node
  com.twitter.common.zookeeper-client

com.twitter.common.zookeeper-map
  com.twitter.common.zookeeper-client

com.twitter.common.zookeeper-lock
  com.twitter.common.zookeeper-client

com.twitter.common.zookeeper-group
  com.twitter.common.zookeeper-client

com.twitter.common.zookeeper-partitioner
  com.twitter.common.zookeeper-group
    com.twitter.common.zookeeper-client

com.twitter.common.zookeeper-candidate
  com.twitter.common.zookeeper-client
  com.twitter.common.zookeeper-group

com.twitter.common.zookeeper-server-set
  com.twitter.common.zookeeper-client
  com.twitter.common.zookeeper-group

com.twitter.common.zookeeper-singleton-service
  com.twitter.common.zookeeper-candidate
    com.twitter.common.zookeeper-client
    com.twitter.common.zookeeper-group
  com.twitter.common.zookeeper-server-set

com.twitter.common-zookeeper-testing
  com.twitter.common.zookeeper-candidate
    com.twitter.common.zookeeper-client
    com.twitter.common.zookeeper-group
  com.twitter.common.zookeeper-lock
  com.twitter.common.zookeeper-map
  com.twitter.common.zookeeper-node
  com.twitter.common.zookeeper-partitioner
  com.twitter.common.zookeeper-server-set
  com.twitter.common.zookeeper-singleton-service
Contributor

jsirois commented Apr 3, 2012

Checking in on this. Did you still have an interest in finishing this off?

jdanbrown referenced this pull request in boundary/ordasity Sep 18, 2012

Closed

What is com.twitter.commons:zookeeper:0.1.1? #8

Contributor

eric commented Jul 31, 2013

I would really love to have this as well.

Contributor

eric commented Aug 1, 2013

Are tests the only thing this is missing to be able to be merged?

@jsirois jsirois added a commit that referenced this pull request Aug 3, 2013

@jsirois jsirois Merge pull request #165 from eric/zookeeper-map-listener
Update to #11: "Add an optional 'Listener' to ZooKeeperMap which fires as nodes are added, changed, or removed"
a2027a2
Contributor

eric commented Aug 3, 2013

This has been merged as #165. 👯

Contributor

jsirois commented Aug 4, 2013

Thanks @cscotta and @eric - closing this out and publishing a jar Monday.

jsirois closed this Aug 4, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment