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

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


+
Skye Wanderman-Milne 2012-11-30, 21:14
Copy link to this message
-
Re: Review Request: ZOOKEEPER-1346: Handle 4lws and monitoring on separate port (creating jetty server)


> 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:
+
Skye Wanderman-Milne 2012-11-16, 23:25
+
Skye Wanderman-Milne 2012-11-30, 21:01
+
Henry Robinson 2012-12-11, 01:25
+
Patrick Hunt 2012-12-14, 17:39
+
Patrick Hunt 2012-12-14, 22:39
+
Skye Wanderman-Milne 2012-12-14, 22:28
+
Skye Wanderman-Milne 2012-12-18, 08:15
+
Skye Wanderman-Milne 2013-01-04, 02:17
+
Camille Fournier 2012-11-18, 16:24
+
Camille Fournier 2012-11-18, 16:21
+
Skye Wanderman-Milne 2012-11-19, 23:55
+
Camille Fournier 2012-11-18, 16:10
+
Skye Wanderman-Milne 2012-11-30, 21:17
+
Skye Wanderman-Milne 2012-11-19, 23:55
+
Skye Wanderman-Milne 2013-01-15, 23:53