----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8094/#review13826 -----------------------------------------------------------
Looking good. src/java/main/org/apache/zookeeper/server/DataTree.java <https://reviews.apache.org/r/8094/#comment29525> Good time to add some Javadoc to some of these public methods. src/java/main/org/apache/zookeeper/server/WatchManager.java <https://reviews.apache.org/r/8094/#comment29526> 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.? src/java/main/org/apache/zookeeper/server/WatchManager.java <https://reviews.apache.org/r/8094/#comment29529> 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. src/java/main/org/apache/zookeeper/server/WatchManager.java <https://reviews.apache.org/r/8094/#comment29528> Nit: avoid the repeated lookup by id and make the HashSet a local variable, then do id2paths.put after the for loop. src/java/main/org/apache/zookeeper/server/ZooKeeperServerMain.java <https://reviews.apache.org/r/8094/#comment29530> 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. src/java/main/org/apache/zookeeper/server/ZooTrace.java <https://reviews.apache.org/r/8094/#comment29531> *If* this is used from multiple threads (is it?), it should be synchronized on the class lock, otherwise there's no guarantee that changes to traceMask are visible on other threads. src/java/main/org/apache/zookeeper/server/admin/AdminServer.java <https://reviews.apache.org/r/8094/#comment29533> I prefer starting servers in a separate method outside of the constructor. That way you can always initialise variables of type AdminServer in their usual place without having to wait until you know you're ok with starting the server before you construct one. src/java/main/org/apache/zookeeper/server/admin/AdminServer.java <https://reviews.apache.org/r/8094/#comment29532> LOG.warn("Problem starting AdminServer", e); Why don't you want to throw the exception? How can clients know if this worked or not? src/java/main/org/apache/zookeeper/server/admin/CommandBase.java <https://reviews.apache.org/r/8094/#comment29541> Why String[]? Prefer List<String> pretty much everywhere. You can use Arrays.asList("name1", "name2") etc. to pass in names. src/java/main/org/apache/zookeeper/server/admin/CommandOutputter.java <https://reviews.apache.org/r/8094/#comment29540> Outputter is a bit clumsy - would Writer work better? src/java/main/org/apache/zookeeper/server/admin/Commands.java <https://reviews.apache.org/r/8094/#comment29535> You don't need this blank line src/java/main/org/apache/zookeeper/server/admin/Commands.java <https://reviews.apache.org/r/8094/#comment29536> final static variables usually have capitalised names, so COMMANDS here, and PRIMARY_NAMES below. src/java/main/org/apache/zookeeper/server/admin/Commands.java <https://reviews.apache.org/r/8094/#comment29538> Not sure we need this src/java/main/org/apache/zookeeper/server/admin/Commands.java <https://reviews.apache.org/r/8094/#comment29542> These guys all need a short comment to explain what they do. src/java/main/org/apache/zookeeper/server/admin/Commands.java <https://reviews.apache.org/r/8094/#comment29543> New line for second } src/java/main/org/apache/zookeeper/server/admin/Commands.java <https://reviews.apache.org/r/8094/#comment29544> You can't lock on factory.getConnections(), since getConnections is entitled to return different objects on different invocations (e.g. what if it copied the cnxns object and returned the copy?) My suggestion: move this logic into ServerCnxnFactory and make it a public method that returns the List<Map<String, Object>>. Same with the other places you follow a similar pattern. src/java/main/org/apache/zookeeper/server/admin/JsonOutputter.java <https://reviews.apache.org/r/8094/#comment29545> printStackTrace is bad because it doesn't use the logging setup. Use LOG.warn("...", e) in all cases. src/java/test/org/apache/zookeeper/server/PrepRequestProcessorTest.java <https://reviews.apache.org/r/8094/#comment29546> Remove this line (and the one above). Shouldn't this return an empty Map instead of null, in case this ever gets used? - Henry Robinson 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 > >
