Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Zookeeper >> mail # dev >> Review Request: ZOOKEEPER-1346: Handle 4lws and monitoring on separate port (creating jetty server)


Copy link to this message
-
Re: Review Request: ZOOKEEPER-1346: Handle 4lws and monitoring on separate port (creating jetty server)


> On Nov. 18, 2012, 4:10 p.m., Camille Fournier wrote:
> > src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java, line 306
> > <https://reviews.apache.org/r/8094/diff/1/?file=190875#file190875line306>
> >
> >     Why make this a more restrictive type?

If you look at ConsCommand or StatCommand in Commands.java, I clone the HashSet returned by getConnections(). I copied this code from the original 4lw's in NIOServerCnxn.java, including the comment that says "clone should be faster than iteration ie give up the cnxns lock faster". The original code clones factory.cnxns directly, but since Commands is in a different package I have to use factory.getCommands(), so I changed the return type in order to use clone.

I could avoid changing the return type by using a copy constructor instead of clone(), but I didn't want to change the command code based on the comment. FWIW, NettyServerCnxn uses a copy constructor instead of clone(), so maybe it's fine.
> On Nov. 18, 2012, 4:10 p.m., Camille Fournier wrote:
> > src/java/main/org/apache/zookeeper/server/WatchManager.java, line 199
> > <https://reviews.apache.org/r/8094/diff/1/?file=190882#file190882line199>
> >
> >     Ditto on dumpWatches with the PrintWriter. Let's have one place for the logic for building up what should be written, and use the built object to print so if we decide to change the logic we only have to change it once.

I totally agree. I forgot to mention it earlier, but for this initial review I left all the original 4lw code untouched (partly so we can focus on just the new code/functionality for now, and partly because reimplementing + refactoring all the original formatting of the 4lw's is a pain :)). I'll start on combining the code and will post it in the next review.
> On Nov. 18, 2012, 4:10 p.m., Camille Fournier wrote:
> > src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java, line 110
> > <https://reviews.apache.org/r/8094/diff/1/?file=190885#file190885line110>
> >
> >     What's with the TODO?

Just a reminder for me to start an AdminServer for each server in a quorum. Right now it only starts in standalone mode.
- Skye
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8094/#review13547
-----------------------------------------------------------
On Nov. 16, 2012, 11:25 p.m., Skye Wanderman-Milne wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8094/
> -----------------------------------------------------------
>
> (Updated Nov. 16, 2012, 11:25 p.m.)
>
>
> Review request for zookeeper, Patrick Hunt, Camille Fournier, and Henry Robinson.
>
>
> Description
> -------
>
> See my comment in ZOOKEEPER-1346.
>
>
> This addresses bug ZOOKEEPER-1346.
>     https://issues.apache.org/jira/browse/ZOOKEEPER-1346
>
>
> Diffs
> -----
>
>   ivy.xml fadf4f4
>   src/java/main/org/apache/zookeeper/server/DataTree.java 0bb2317
>   src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java 8b4c46b
>   src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java eec2f2a
>   src/java/main/org/apache/zookeeper/server/ServerCnxn.java 6dd509b
>   src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java e5c6565
>   src/java/main/org/apache/zookeeper/server/ServerStats.java aa0d93f
>   src/java/main/org/apache/zookeeper/server/SessionTracker.java 3535e1b
>   src/java/main/org/apache/zookeeper/server/SessionTrackerImpl.java 31f2785
>   src/java/main/org/apache/zookeeper/server/WatchManager.java 0e7c815
>   src/java/main/org/apache/zookeeper/server/ZKDatabase.java d6c0c05
>   src/java/main/org/apache/zookeeper/server/ZooKeeperServer.java 7bb7b2f
>   src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 369e621
>   src/java/main/org/apache/zookeeper/server/ZooTrace.java ac14fe2