----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33339/#review86113 -----------------------------------------------------------
This is great -sorry it took so long to get to do a review. Thanks for doing it, I'm quite looking forward to using it to learning more about the Persistent Framework :) it would be great if we could have a bit more comments in the code to help all other newbies. My review was fairly narrowly focused on Java style etc. - I'm expecting one of the commiters will be able to give you feedback about using the framework. src/examples/java/TestPersistentVolumeFramework.java <https://reviews.apache.org/r/33339/#comment138014> as this is an "example" framework, it'd be great if it could have (extensive) javadocs: newbies and users alike are likely to look inside here first to learn (I sure did :) and providing them with good, extensive guidance as to why stuff is done the way it is, it would be great! Ideally, all public classes + public methods should have javadocs (not sure if this is also in the public google java style guide, it sure was a "MUST" in our internal one) Thanks! src/examples/java/TestPersistentVolumeFramework.java <https://reviews.apache.org/r/33339/#comment138022> please add (at the very least) the `@param` docs so we know what these lists are about and how they're going to be used. also, as you are modifying `remains` you may want to alert the users of your method to that. src/examples/java/TestPersistentVolumeFramework.java <https://reviews.apache.org/r/33339/#comment138015> please avoid this; use instead the `foreach` pattern: ``` for (Resource require : requires) { ... ``` src/examples/java/TestPersistentVolumeFramework.java <https://reviews.apache.org/r/33339/#comment138016> here too src/examples/java/TestPersistentVolumeFramework.java <https://reviews.apache.org/r/33339/#comment138017> this is really hard to 'parse'; also I don't really like to see so many `continue` makes it difficult to follow the flow of the loop Could we please reverse the condition and have the pattern of: ``` if (a && b &&c) { // do something } ``` this also forces you to have a `break` (I think?) further down. Following the logic of this nested for-loop is really hard. src/examples/java/TestPersistentVolumeFramework.java <https://reviews.apache.org/r/33339/#comment138018> here too src/examples/java/TestPersistentVolumeFramework.java <https://reviews.apache.org/r/33339/#comment138019> do you really need the `else-continue` here? src/examples/java/TestPersistentVolumeFramework.java <https://reviews.apache.org/r/33339/#comment138020> a better name for `remain` would be something like `consumed` or `allocated` src/examples/java/TestPersistentVolumeFramework.java <https://reviews.apache.org/r/33339/#comment138025> can we refactor this very complex comparison into its own (aptly named) method? (with some javadoc too :) src/examples/java/TestPersistentVolumeFramework.java <https://reviews.apache.org/r/33339/#comment138028> I find the pattern of returning a String in case of error and `null` for success a bit un-Java-like. how about having some form of an `Status` class (I mean, you may even consider a checked `ApplyResourceException`) that takes a string as an initializer and has an `ok()` method? as an aside: given the method below that takes a List<Operation>, I would make this `private` so that clients don't have to think about which one to use (even if they have only ONE op to apply): ``` applyResource(res, Lists.newImmutableList(oneOpOnly)); ``` cleaner API, IMO. src/examples/java/TestPersistentVolumeFramework.java <https://reviews.apache.org/r/33339/#comment138029> unless I'm mistaken and this is a unit test (is it?) can we please rename this class in a more meaningful way? TestXxxx should be reserved for test classes. src/examples/java/TestPersistentVolumeFramework.java <https://reviews.apache.org/r/33339/#comment138030> instead of System.out, please consider using a java.logging framework (either log4j or any of the other variants): it's low effort and makes the interface much cleaner and more extensible src/examples/java/TestPersistentVolumeFramework.java <https://reviews.apache.org/r/33339/#comment138034> you see? here, you would have: ``` Status status = applyResource(...); if (!status.ok()) { log.error("This went so horribly wrong: ", status.errMsg()); return; } ``` or something to that effect src/examples/java/TestPersistentVolumeFramework.java <https://reviews.apache.org/r/33339/#comment138036> shouldn't you `.build()` here? also, a better pattern is to use `.toString()` (as opposed to force a conversion via `+`) - if that's what you are doing (is it? sorry if I got this one wrong!) src/examples/java/TestPersistentVolumeFramework.java <https://reviews.apache.org/r/33339/#comment138038> this is almost identical code to L307-315: how about factoring out to a common `buildTaskInfo(String command)`? src/examples/java/TestPersistentVolumeFramework.java <https://reviews.apache.org/r/33339/#comment138039> instead of concatenating string, either use a StringBuilder or, even better: ``` String.format("Received framework message from executor '%s' on slave %s: '%s'", executorId, slaveId, data); ``` src/examples/java/TestPersistentVolumeFramework.java <https://reviews.apache.org/r/33339/#comment138040> here too, please src/examples/java/TestPersistentVolumeFramework.java <https://reviews.apache.org/r/33339/#comment138043> Please consider using Apache Commons CLI instead: https://commons.apache.org/proper/commons-cli/ src/examples/java/TestPersistentVolumeFramework.java <https://reviews.apache.org/r/33339/#comment138044> if (master.isEmpty()) ... - Marco Massenzio On May 18, 2015, 4:42 p.m., haosdent huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33339/ > ----------------------------------------------------------- > > (Updated May 18, 2015, 4:42 p.m.) > > > Review request for mesos, Jie Yu and Vinod Kone. > > > Bugs: MESOS-2610 > https://issues.apache.org/jira/browse/MESOS-2610 > > > Repository: mesos > > > Description > ------- > > Add a Java example framework to test persistent volumes. > > > Diffs > ----- > > configure.ac 7f9e52916b9d78f2bbff9d6ed9871444a0fda629 > src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b > src/examples/java/TestPersistentVolumeFramework.java PRE-CREATION > src/examples/java/test-persistent-volume-framework.in PRE-CREATION > src/tests/examples_tests.cpp f85b81562158c5499e9804d8d7b6811bb0a3ef16 > src/tests/java_persistent_volume_framework_test.sh PRE-CREATION > > Diff: https://reviews.apache.org/r/33339/diff/ > > > Testing > ------- > > make check > > > Thanks, > > haosdent huang > >
