Zookeeper, mail # dev - Re: Review Request: ZOOKEEPER-1346: Handle 4lws and monitoring on separate port (creating jetty server)

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

Skye Wanderman-Milne 2012-11-30, 21:14

> On Nov. 28, 2012, 6:19 p.m., Patrick Hunt wrote:
> > src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java, line 111
> > <https://reviews.apache.org/r/8094/diff/1/?file=190885#file190885line111>
> >
> > This needs to be configurable from the get go.
>
> Skye Wanderman-Milne wrote:
> I'll add configuration options for the base URL, port, anything else I can think of.

Added system properties for port, base URL, and commands URL.
- Skye
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8094/#review13824
-----------------------------------------------------------
On Nov. 30, 2012, 9:01 p.m., Skye Wanderman-Milne wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8094/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2012, 9:01 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
> src/java/main/org/apache/zookeeper/server/admin/AdminServer.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/Command.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/CommandBase.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/CommandOutputter.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/Commands.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/JsonOutputter.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/quorum/Leader.java 4d09b43
> src/java/main/org/apache/zookeeper/server/quorum/Learner.java e8d548b
> src/java/main/org/apache/zookeeper/server/quorum/LearnerSessionTracker.java 3182419
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 4e3a87d
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java deae926
> src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java d3f1492
> src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java 8665bac
> src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java 42207e1
>
> Diff: https://reviews.apache.org/r/8094/diff/
>
>
> Testing
> -------
>
> unit tests
>
> Ran in standalone mode (only option right now) and manually tried out all the commands/links
>
>
> Thanks,
>
> Skye Wanderman-Milne
>
>

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

Skye Wanderman-Milne 2012-11-30, 21:13

> On Nov. 28, 2012, 7:32 p.m., Henry Robinson wrote:
> > src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java, line 110
> > <https://reviews.apache.org/r/8094/diff/1/?file=190885#file190885line110>
> >
> > Can you expand the TODO to be clearer about what you want to happen? It might not be you that fixes it, such is open source.

I fixed it :)
> On Nov. 28, 2012, 7:32 p.m., Henry Robinson wrote:
> > src/java/main/org/apache/zookeeper/server/WatchManager.java, line 176
> > <https://reviews.apache.org/r/8094/diff/1/?file=190882#file190882line176>
> >
> > I'm not a huge fan of calling these 'dump' - that always implies writing to a string or to stderr or similar. How about just 'getWatches' etc.?
> On Nov. 28, 2012, 7:32 p.m., Henry Robinson wrote:
> > src/java/main/org/apache/zookeeper/server/WatchManager.java, line 179
> > <https://reviews.apache.org/r/8094/diff/1/?file=190882#file190882line179>
> >
> > Nit: You're boxing the long every time you do a get or a put, you might as well make this a Long id instead.
> On Nov. 28, 2012, 7:32 p.m., Henry Robinson wrote:
> > src/java/main/org/apache/zookeeper/server/WatchManager.java, line 182
> > <https://reviews.apache.org/r/8094/diff/1/?file=190882#file190882line182>
> >
> > Nit: avoid the repeated lookup by id and make the HashSet a local variable, then do id2paths.put after the for loop.
> On Nov. 28, 2012, 7:32 p.m., Henry Robinson wrote:
> > src/java/main/org/apache/zookeeper/server/admin/CommandBase.java, line 35
> > <https://reviews.apache.org/r/8094/diff/1/?file=190889#file190889line35>
> >
> > Why String[]? Prefer List<String> pretty much everywhere. You can use Arrays.asList("name1", "name2") etc. to pass in names.

No good reason, I like using arrays where I would use tuples in python :) Fixed.
> On Nov. 28, 2012, 7:32 p.m., Henry Robinson wrote:
> > src/java/main/org/apache/zookeeper/server/admin/Commands.java, line 57
> > <https://reviews.apache.org/r/8094/diff/1/?file=190891#file190891line57>
> >
> > You don't need this blank line
> On Nov. 28, 2012, 7:32 p.m., Henry Robinson wrote:
> > src/java/main/org/apache/zookeeper/server/admin/Commands.java, line 61
> > <https://reviews.apache.org/r/8094/diff/1/?file=190891#file190891line61>
> >
> > final static variables usually have capitalised names, so COMMANDS here, and PRIMARY_NAMES below.

I removed the final modifier because I didn't want to capitalize the variable names :) I generally use capitals to denote _immutable_ static final variables (or at least variables that should be treated as immutable). commands and primaryNames are modified when you register a Command so I think it'd be confusing to have them in all caps. Personally I like marking variables as final when possible so the compiler reminds you to initialize them, even if they're mutable, but it's not necessary.
> On Nov. 28, 2012, 7:32 p.m., Henry Robinson wrote:
> > src/java/main/org/apache/zookeeper/server/admin/Commands.java, line 111
> > <https://reviews.apache.org/r/8094/diff/1/?file=190891#file190891line111>
> >
> > Not sure we need this
> On Nov. 28, 2012, 7:32 p.m., Henry Robinson wrote:
> > src/java/main/org/apache/zookeeper/server/admin/Commands.java, line 159
> > <https://reviews.apache.org/r/8094/diff/1/?file=190891#file190891line159>
> >
> > New line for second }
> On Nov. 28, 2012, 7:32 p.m., Henry Robinson wrote:
> > src/java/main/org/apache/zookeeper/server/admin/JsonOutputter.java, line 58
> > <https://reviews.apache.org/r/8094/diff/1/?file=190892#file190892line58>
> >
> > printStackTrace is bad because it doesn't use the logging setup. Use LOG.warn("...", e) in all cases.
> On Nov. 28, 2012, 7:32 p.m., Henry Robinson wrote:
> > src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java, line 128
> > <https://reviews.apache.org/r/8094/diff/1/?file=190894#file190894line128>
> >
> > Remove this line (and the one above). Shouldn't this return an empty Map instead of null, in case this ever gets used?

Added comments that give a short description and go over what's returned by the command (i.e., what keys the returned Map will have). Commands also have a String doc field, but I'm thinking of getting rid of it because I'm not sure where/how to use it. I think it will be better to put each Command's API in the ZK docs somewhere.

Added synchronization, a bunch of threads call the functions accessing traceMask.

I created an AdminServerException, which is thrown instead of catching the Exception.

CommandWriter sounds like it should be a subclass of Writer (a la PrintWriter, StringWriter, etc.). Maybe CommandPrinter?
- Skye
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8094/#review13826
On Nov. 30, 2012, 9:01 p.m., Skye Wanderman-Milne wrote:

Review Request: ZOOKEEPER-1346: Handle 4lws and monitoring on separate port (creating jetty server)

Skye Wanderman-Milne 2012-11-16, 23:25

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

Skye Wanderman-Milne 2012-11-30, 21:01

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8094/
-----------------------------------------------------------

(Updated Nov. 30, 2012, 9:01 p.m.)
Review request for zookeeper, Patrick Hunt, Camille Fournier, and Henry Robinson.
Changes
-------

This patch should address all the comments EXCEPT adding a new CommandOutputter that outputs the original unstructured text and refactoring the existing 4lw's to use Commands. I'll upload a new patch with those additions soon; I'm trying to keep these reviews somewhat manageable.

There are a lot of changes here but most of them are small (comments, renaming, minor refactoring, etc.). The biggest change is how I start the AdminServer. I added AdminServer.setZooKeeperServer, and create the AdminServer before creating a ZooKeeperServer. If no server is set, commands will fail with error "This ZooKeeper instance is not currently serving requests". This duplicates the functionality of the current 4lws. An AdminServer is also started in QuorumPeerMain now, not just ZooKeeperServerMain.
Description
-------

See my comment in ZOOKEEPER-1346.
This addresses bug ZOOKEEPER-1346.
   https://issues.apache.org/jira/browse/ZOOKEEPER-1346
Diffs (updated)
-----

  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
  src/java/main/org/apache/zookeeper/server/admin/AdminServer.java PRE-CREATION
  src/java/main/org/apache/zookeeper/server/admin/Command.java PRE-CREATION
  src/java/main/org/apache/zookeeper/server/admin/CommandBase.java PRE-CREATION
  src/java/main/org/apache/zookeeper/server/admin/CommandOutputter.java PRE-CREATION
  src/java/main/org/apache/zookeeper/server/admin/Commands.java PRE-CREATION
  src/java/main/org/apache/zookeeper/server/admin/JsonOutputter.java PRE-CREATION
  src/java/main/org/apache/zookeeper/server/quorum/Leader.java 4d09b43
  src/java/main/org/apache/zookeeper/server/quorum/Learner.java e8d548b
  src/java/main/org/apache/zookeeper/server/quorum/LearnerSessionTracker.java 3182419
  src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 4e3a87d
  src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java deae926
  src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java d3f1492
  src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java 8665bac
  src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java 42207e1

Diff: https://reviews.apache.org/r/8094/diff/
Testing
-------

unit tests

Ran in standalone mode (only option right now) and manually tried out all the commands/links
Thanks,

Skye Wanderman-Milne

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

Henry Robinson 2012-12-11, 01:25

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8094/#review14281
-----------------------------------------------------------

Ship it!
Looks good to me.
src/java/main/org/apache/zookeeper/server/DataTree.java
<https://reviews.apache.org/r/8094/#comment30415>

   Long, not long

src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java
<https://reviews.apache.org/r/8094/#comment30417>

   fwiw, I bet the difference is completely negligible. (I know this isn't your comment). No need to change it though.

src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java
<https://reviews.apache.org/r/8094/#comment30416>

   space after )

src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java
<https://reviews.apache.org/r/8094/#comment30418>

   Space after )

src/java/main/org/apache/zookeeper/server/admin/Commands.java
<https://reviews.apache.org/r/8094/#comment30421>

   Oh for Guava, and ImmutableMap.of(..) or something like it.
- Henry Robinson
On Nov. 30, 2012, 9:01 p.m., Skye Wanderman-Milne wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8094/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2012, 9:01 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
> src/java/main/org/apache/zookeeper/server/admin/AdminServer.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/Command.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/CommandBase.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/CommandOutputter.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/Commands.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/JsonOutputter.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/quorum/Leader.java 4d09b43
> src/java/main/org/apache/zookeeper/server/quorum/Learner.java e8d548b
> src/java/main/org/apache/zookeeper/server/quorum/LearnerSessionTracker.java 3182419
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 4e3a87d
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java deae926
> src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java d3f1492
> src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java 8665bac
> src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java 42207e1
>
> Diff: https://reviews.apache.org/r/8094/diff/
>
>
> Testing
> -------
>
> unit tests
>
> Ran in standalone mode (only option right now) and manually tried out all the commands/links

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

Patrick Hunt 2012-12-14, 17:39

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8094/#review14507
-----------------------------------------------------------
Noticed some issues with configuration and documentation. The system properties should be moved into the config file. Also we need to document this feature and the configuration.

At the very least basic unit tests need to be added.

It would be good to attach the latest version of the patch to ZOOKEEPER-1346 and "submit" the jira so that qabot can take a pass on it.

src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java
<https://reviews.apache.org/r/8094/#comment30896>

whether or not the server is started should be made configurable. On by default is fine.

src/java/main/org/apache/zookeeper/server/admin/AdminServer.java
<https://reviews.apache.org/r/8094/#comment30897>

Configurable properties such as this need to be in the config file, not a system property.
- Patrick Hunt
On Nov. 30, 2012, 9:01 p.m., Skye Wanderman-Milne wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8094/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2012, 9:01 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
> src/java/main/org/apache/zookeeper/server/admin/AdminServer.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/Command.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/CommandBase.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/CommandOutputter.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/Commands.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/JsonOutputter.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/quorum/Leader.java 4d09b43
> src/java/main/org/apache/zookeeper/server/quorum/Learner.java e8d548b
> src/java/main/org/apache/zookeeper/server/quorum/LearnerSessionTracker.java 3182419
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 4e3a87d
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java deae926
> src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java d3f1492
> src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java 8665bac
> src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java 42207e1
>
> Diff: https://reviews.apache.org/r/8094/diff/
>
>
> Testing
> -------
>
> unit tests
>
> Ran in standalone mode (only option right now) and manually tried out all the commands/links
>
>

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

Patrick Hunt 2012-12-14, 22:39

> On Dec. 14, 2012, 5:39 p.m., Patrick Hunt wrote:
> > src/java/main/org/apache/zookeeper/server/admin/AdminServer.java, lines 56-58
> > <https://reviews.apache.org/r/8094/diff/2/?file=232264#file232264line56>
> >
> > Configurable properties such as this need to be in the config file, not a system property.
>
> Skye Wanderman-Milne wrote:
> System properties can be set via the config file; see QuorumPeerConfig.java:206.

You're right - honestly I've never noticed that one before!
- Patrick
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8094/#review14507
-----------------------------------------------------------
On Nov. 30, 2012, 9:01 p.m., Skye Wanderman-Milne wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8094/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2012, 9:01 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
> src/java/main/org/apache/zookeeper/server/admin/AdminServer.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/Command.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/CommandBase.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/CommandOutputter.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/Commands.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/JsonOutputter.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/quorum/Leader.java 4d09b43
> src/java/main/org/apache/zookeeper/server/quorum/Learner.java e8d548b
> src/java/main/org/apache/zookeeper/server/quorum/LearnerSessionTracker.java 3182419
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 4e3a87d
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java deae926
> src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java d3f1492
> src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java 8665bac
> src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java 42207e1
>
> Diff: https://reviews.apache.org/r/8094/diff/
>
>
> Testing
> -------
>
> unit tests
>
> Ran in standalone mode (only option right now) and manually tried out all the commands/links
>
>
> Thanks,
>
> Skye Wanderman-Milne
>
>

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

Skye Wanderman-Milne 2012-12-14, 22:28

> On Dec. 14, 2012, 5:39 p.m., Patrick Hunt wrote:
> > src/java/main/org/apache/zookeeper/server/admin/AdminServer.java, lines 56-58
> > <https://reviews.apache.org/r/8094/diff/2/?file=232264#file232264line56>
> >
> > Configurable properties such as this need to be in the config file, not a system property.

System properties can be set via the config file; see QuorumPeerConfig.java:206.
- Skye
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8094/#review14507
-----------------------------------------------------------
On Nov. 30, 2012, 9:01 p.m., Skye Wanderman-Milne wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8094/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2012, 9:01 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
> src/java/main/org/apache/zookeeper/server/admin/AdminServer.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/Command.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/CommandBase.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/CommandOutputter.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/Commands.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/JsonOutputter.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/quorum/Leader.java 4d09b43
> src/java/main/org/apache/zookeeper/server/quorum/Learner.java e8d548b
> src/java/main/org/apache/zookeeper/server/quorum/LearnerSessionTracker.java 3182419
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 4e3a87d
> src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java deae926
> src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java d3f1492
> src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java 8665bac
> src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java 42207e1
>
> Diff: https://reviews.apache.org/r/8094/diff/
>
>
> Testing
> -------
>
> unit tests
>
> Ran in standalone mode (only option right now) and manually tried out all the commands/links
>
>
> Thanks,
>
> Skye Wanderman-Milne
>
>

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

Skye Wanderman-Milne 2012-12-18, 08:15

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8094/
-----------------------------------------------------------

(Updated Dec. 18, 2012, 8:14 a.m.)
Review request for zookeeper, Patrick Hunt, Camille Fournier, and Henry Robinson.
Changes
-------

Added CommandsTest, documentation to the ZooKeeper Admin's Guide, rebased on trunk, small fixes/improvements from comments.

CommandsTest verifies that each of the Commands outputs a Map with the expected keys and value types, the idea being to prevent accidental deviation from the specified API. It also runs each command and checks that there's no error. Does anyone have other ideas re: what to test?

The documentation in the Admin's Guide is a little sparse right now, but I'll add more once all the planned features are in.
Description
-------

See my comment in ZOOKEEPER-1346.
This addresses bug ZOOKEEPER-1346.
   https://issues.apache.org/jira/browse/ZOOKEEPER-1346
Diffs (updated)
-----

  ivy.xml fadf4f4
  src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml 47190a8
  src/java/main/org/apache/zookeeper/server/DataTree.java 0bb2317
  src/java/main/org/apache/zookeeper/server/NIOServerCnxn.java eade1d6
  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 cbe35fd
  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
  src/java/main/org/apache/zookeeper/server/admin/AdminServer.java PRE-CREATION
  src/java/main/org/apache/zookeeper/server/admin/Command.java PRE-CREATION
  src/java/main/org/apache/zookeeper/server/admin/CommandBase.java PRE-CREATION
  src/java/main/org/apache/zookeeper/server/admin/CommandOutputter.java PRE-CREATION
  src/java/main/org/apache/zookeeper/server/admin/Commands.java PRE-CREATION
  src/java/main/org/apache/zookeeper/server/admin/JsonOutputter.java PRE-CREATION
  src/java/main/org/apache/zookeeper/server/quorum/Leader.java 8a432ff
  src/java/main/org/apache/zookeeper/server/quorum/Learner.java e8d548b
  src/java/main/org/apache/zookeeper/server/quorum/LearnerSessionTracker.java 3182419
  src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 9c17f5e
  src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java deae926
  src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java d3f1492
  src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java 8665bac
  src/java/test/org/apache/zookeeper/server/admin/CommandsTest.java PRE-CREATION
  src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java 370e7bb
  src/java/test/org/apache/zookeeper/test/ClientBase.java 94f1cb0

Diff: https://reviews.apache.org/r/8094/diff/
Testing
-------

unit tests

Ran in standalone mode (only option right now) and manually tried out all the commands/links
Thanks,

Skye Wanderman-Milne

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

Skye Wanderman-Milne 2013-01-04, 02:17

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8094/
-----------------------------------------------------------

(Updated Jan. 4, 2013, 2:17 a.m.)
Review request for zookeeper, Patrick Hunt, Camille Fournier, and Henry Robinson.
Changes
-------

Rebased on trunk again (Unfortunately this required some pretty large changes, making the diff between this patch and the previous one hard to read.)

Made it possible to disable the AdminServer either by setting the zookeeper.admin.enableAdminServer system property to false or by removing jetty from the classpath (in case users don't want to depend on Jetty). I implemented this by extracting AdminServer into an interface with two subclasses, JettyAdminServer (the original implementation) and DummyAdminServer (which does nothing and is used when the server is disabled). AdminServerFactory is then responsible for creating the appropriate server. I updated the documentation to reflect this.

Using the system property, I disabled the AdminServer during tests as it was causing some tests to hang when they tried to start multiple AdminServers on the same port. I also added some more comments and renamed some functions to make them clearer.
Description
-------

See my comment in ZOOKEEPER-1346.
This addresses bug ZOOKEEPER-1346.
   https://issues.apache.org/jira/browse/ZOOKEEPER-1346
Diffs (updated)
-----

  ivy.xml fadf4f4
  src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml 47190a8
  src/java/main/org/apache/zookeeper/server/DataTree.java d6c7773
  src/java/main/org/apache/zookeeper/server/ExpiryQueue.java 130c58e
  src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java 9422538
  src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java eec2f2a
  src/java/main/org/apache/zookeeper/server/ServerCnxn.java 35ea301
  src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java 267dbdf
  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 137862e
  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 14e754b
  src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 369e621
  src/java/main/org/apache/zookeeper/server/ZooTrace.java ac14fe2
  src/java/main/org/apache/zookeeper/server/admin/AdminServer.java PRE-CREATION
  src/java/main/org/apache/zookeeper/server/admin/AdminServerFactory.java PRE-CREATION
  src/java/main/org/apache/zookeeper/server/admin/Command.java PRE-CREATION
  src/java/main/org/apache/zookeeper/server/admin/CommandBase.java PRE-CREATION
  src/java/main/org/apache/zookeeper/server/admin/CommandOutputter.java PRE-CREATION
  src/java/main/org/apache/zookeeper/server/admin/Commands.java PRE-CREATION
  src/java/main/org/apache/zookeeper/server/admin/DummyAdminServer.java PRE-CREATION
  src/java/main/org/apache/zookeeper/server/admin/JettyAdminServer.java PRE-CREATION
  src/java/main/org/apache/zookeeper/server/admin/JsonOutputter.java PRE-CREATION
  src/java/main/org/apache/zookeeper/server/quorum/Leader.java 8a432ff
  src/java/main/org/apache/zookeeper/server/quorum/Learner.java e8d548b
  src/java/main/org/apache/zookeeper/server/quorum/LearnerSessionTracker.java 3182419
  src/java/main/org/apache/zookeeper/server/quorum/QuorumPeer.java 9c17f5e
  src/java/main/org/apache/zookeeper/server/quorum/QuorumPeerMain.java deae926
  src/java/main/org/apache/zookeeper/server/quorum/ReadOnlyZooKeeperServer.java d3f1492
  src/java/test/org/apache/zookeeper/ZKTestCase.java 6ecfe8f
  src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java 8665bac
  src/java/test/org/apache/zookeeper/server/admin/CommandsTest.java PRE-CREATION
  src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java 370e7bb

Diff: https://reviews.apache.org/r/8094/diff/
Testing

unit tests

Ran in standalone mode (only option right now) and manually tried out all the commands/links
Thanks,

Skye Wanderman-Milne

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

Camille Fournier 2012-11-18, 16:24

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8094/#review13549
-----------------------------------------------------------
Those are just some initial comments (on a spotty connection on a train). Looks like a great start though.

- Camille Fournier
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
> src/java/main/org/apache/zookeeper/server/admin/AdminServer.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/Command.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/CommandBase.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/CommandOutputter.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/Commands.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/JsonOutputter.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/quorum/LearnerSessionTracker.java 3182419
> src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java 8665bac
> src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java 42207e1
>
> Diff: https://reviews.apache.org/r/8094/diff/
>
>
> Testing
> -------
>
> unit tests
>
> Ran in standalone mode (only option right now) and manually tried out all the commands/links
>
>
> Thanks,
>
> Skye Wanderman-Milne
>
>

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

Camille Fournier 2012-11-18, 16:21

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8094/#review13548
-----------------------------------------------------------

src/java/main/org/apache/zookeeper/server/admin/Commands.java
<https://reviews.apache.org/r/8094/#comment29112>

Why use a LinkedHashMap here?
- Camille Fournier
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
> src/java/main/org/apache/zookeeper/server/admin/AdminServer.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/Command.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/CommandBase.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/CommandOutputter.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/Commands.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/JsonOutputter.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/quorum/LearnerSessionTracker.java 3182419
> src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java 8665bac
> src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java 42207e1
>
> Diff: https://reviews.apache.org/r/8094/diff/
>
>
> Testing
> -------
>
> unit tests
>
> Ran in standalone mode (only option right now) and manually tried out all the commands/links
>
>
> Thanks,
>
> Skye Wanderman-Milne
>
>

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

Skye Wanderman-Milne 2012-11-19, 23:55

> On Nov. 18, 2012, 4:21 p.m., Camille Fournier wrote:
> > src/java/main/org/apache/zookeeper/server/admin/Commands.java, line 94
> > <https://reviews.apache.org/r/8094/diff/1/?file=190891#file190891line94>
> >
> > Why use a LinkedHashMap here?

No reason, I'll change it to a HashMap. (I originally displayed the commands on the /commands page in the order they were registered, but then I changed it to alphabetical order.)
- Skye
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8094/#review13548
-----------------------------------------------------------
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
> src/java/main/org/apache/zookeeper/server/admin/AdminServer.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/Command.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/CommandBase.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/CommandOutputter.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/Commands.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/JsonOutputter.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/quorum/LearnerSessionTracker.java 3182419
> src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java 8665bac
> src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java 42207e1
>
> Diff: https://reviews.apache.org/r/8094/diff/
>
>
> Testing
> -------
>
> unit tests
>
> Ran in standalone mode (only option right now) and manually tried out all the commands/links
>
>
> Thanks,
>
> Skye Wanderman-Milne
>
>

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

Camille Fournier 2012-11-18, 16:10

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8094/#review13547
-----------------------------------------------------------

src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java
<https://reviews.apache.org/r/8094/#comment29108>

   Why make this a more restrictive type?

src/java/main/org/apache/zookeeper/server/SessionTrackerImpl.java
<https://reviews.apache.org/r/8094/#comment29109>

   Maybe you should rewrite the dumpSessions that uses the PrintWriter to print the output of this method instead of duplicating some of the logic

src/java/main/org/apache/zookeeper/server/WatchManager.java
<https://reviews.apache.org/r/8094/#comment29110>

   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.

src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java
<https://reviews.apache.org/r/8094/#comment29111>

   What's with the TODO?
- Camille Fournier
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
> src/java/main/org/apache/zookeeper/server/admin/AdminServer.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/Command.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/CommandBase.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/CommandOutputter.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/Commands.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/JsonOutputter.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/quorum/LearnerSessionTracker.java 3182419
> src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java 8665bac
> src/java/test/org/apache/zookeeper/server/quorum/Zab1_0Test.java 42207e1
>
> Diff: https://reviews.apache.org/r/8094/diff/
>
>
> Testing
> -------
>
> unit tests
>
> Ran in standalone mode (only option right now) and manually tried out all the commands/links
>
>
> Thanks,
>
> Skye Wanderman-Milne
>
>

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

Skye Wanderman-Milne 2012-11-30, 21:17

> 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?
>
> Skye Wanderman-Milne wrote:
> 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.
>
> Patrick Hunt wrote:
> You might not want to change this for b/w compat reasons, all else being equal. It's not part of the "public" api, however some teams interact with factories, esp in their test code, and changing this might cause problems for them.
>
> Skye Wanderman-Milne wrote:
> I'll preserve the API and use a copy ctor instead of clone, then.
>
> Henry Robinson wrote:
> Just saw this conversation - I suggest preserving the API but doing the work of the commands inside the ServerCnxnFactory. You might be able to avoid the copy completely this way.

Good idea, fixed.
- Skye
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8094/#review13547
-----------------------------------------------------------
On Nov. 30, 2012, 9:01 p.m., Skye Wanderman-Milne wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8094/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2012, 9:01 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
> src/java/main/org/apache/zookeeper/server/admin/AdminServer.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/Command.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/CommandBase.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/CommandOutputter.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/Commands.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/JsonOutputter.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/quorum/Leader.java 4d09b43

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

Skye Wanderman-Milne 2012-11-19, 23:55

> 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

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

Skye Wanderman-Milne 2013-01-15, 23:53

> On Jan. 9, 2013, 7:13 p.m., Patrick Hunt wrote:
> > src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml, line 1210
> > <https://reviews.apache.org/r/8094/diff/4/?file=244377#file244377line1210>
> >
> > is there a configuration option to disable this feature? I don't see any doc details on this...
> >
> > admin.enableAdminServer seems to be missing?

I somehow managed to document admin.enableAdminServer in my latest patch to the JIRA but not here... regardless it's there now.
> On Jan. 9, 2013, 7:13 p.m., Patrick Hunt wrote:
> > src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml, line 1211
> > <https://reviews.apache.org/r/8094/diff/4/?file=244377#file244377line1211>
> >
> > this should be "New in 3.5.0", won't get into a fix release.

Done.
> On Jan. 9, 2013, 7:13 p.m., Patrick Hunt wrote:
> > src/docs/src/documentation/content/xdocs/zookeeperAdmin.xml, line 1234
> > <https://reviews.apache.org/r/8094/diff/4/?file=244377#file244377line1234>
> >
> > isn't "/" root? Not sure what you mean by "for now nothing is served directly from the root"

Hmm not sure if root is or isn't the right term here.

I mean right now everything happens at "/commands" (by default) and "/" is a 404. I'm tempted to remove this option altogether and only have the command URL option for simplicity.
- Skye
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8094/#review15204
-----------------------------------------------------------
On Jan. 4, 2013, 2:17 a.m., Skye Wanderman-Milne wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8094/
> -----------------------------------------------------------
>
> (Updated Jan. 4, 2013, 2:17 a.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/docs/src/documentation/content/xdocs/zookeeperAdmin.xml 47190a8
> src/java/main/org/apache/zookeeper/server/DataTree.java d6c7773
> src/java/main/org/apache/zookeeper/server/ExpiryQueue.java 130c58e
> src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java 9422538
> src/java/main/org/apache/zookeeper/server/NettyServerCnxnFactory.java eec2f2a
> src/java/main/org/apache/zookeeper/server/ServerCnxn.java 35ea301
> src/java/main/org/apache/zookeeper/server/ServerCnxnFactory.java 267dbdf
> 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 137862e
> 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 14e754b
> src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java 369e621
> src/java/main/org/apache/zookeeper/server/ZooTrace.java ac14fe2
> src/java/main/org/apache/zookeeper/server/admin/AdminServer.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/AdminServerFactory.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/Command.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/CommandBase.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/CommandOutputter.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/Commands.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/DummyAdminServer.java PRE-CREATION
> src/java/main/org/apache/zookeeper/server/admin/JettyAdminServer.java PRE-CREATION