----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33339/#review95611 -----------------------------------------------------------
src/examples/java/TestPersistentVolumeFramework.java (lines 535 - 537) <https://reviews.apache.org/r/33339/#comment150688> here (and everywhere else) can you please explain what the arguments are and, even, what an example use would be? this would be great for folks planning to use this framework as a 'template' for their own. src/examples/java/TestPersistentVolumeFramework.java (line 560) <https://reviews.apache.org/r/33339/#comment150689> why `8`? what does this 'magic number' represent? is this something that should be a CONSTANT or a user/configuration selectable? please also add a comment as to what is going on here. src/examples/java/TestPersistentVolumeFramework.java (line 569) <https://reviews.apache.org/r/33339/#comment150690> s/by/for src/examples/java/TestPersistentVolumeFramework.java (line 574) <https://reviews.apache.org/r/33339/#comment150695> this is really *not* a getter - and I agree that `createCreateOperation` sounds awful, but I guess that we don't much have a choice... Also, you may want to explain in the @param clause that the generic Resource here, really should be a Volume (create, supposedly, in the `getShardPersistentVolume` maybe?) same below for `getLaunchOperation` IMO you can conflate the two into one factory method: ``` public static Offer.Operation createOperation( Operation.Type type, Resource volume, TaskInfo task) { Operation.Builder builder = Operation.newBuilder() .setType(type); switch (type) { case Operation.Type.LAUNCH: builder.setLaunch(....); break; ... } return builder.build(); } ``` or something along those lines. (and does it really need to be a `static` method?) src/examples/java/TestPersistentVolumeFramework.java (line 603) <https://reviews.apache.org/r/33339/#comment150697> please lose the `Test` moniker - everyone will assume this is a unit test and will be scratching their head. Also, please move to its own class file. src/examples/java/TestPersistentVolumeFramework.java (line 622) <https://reviews.apache.org/r/33339/#comment150698> As I mentioned earlier (and I must stress, I feel pretty strongly about this :) we should use a Logger here (and everywhere) instead of System.out AND use something like log4j or whatever you feel comfortable with - and have a log4j.properties that folks can fiddle with and configure logging as they see fit. This is just Java good practice and will set a good example. src/examples/java/TestPersistentVolumeFramework.java (lines 647 - 651) <https://reviews.apache.org/r/33339/#comment150699> this is going to be VERY verbose - replace with a log.debug(...) src/examples/java/TestPersistentVolumeFramework.java (line 657) <https://reviews.apache.org/r/33339/#comment150700> use `offers.size()` to dimension your new List (unless you expect it to grow?) src/examples/java/TestPersistentVolumeFramework.java (line 661) <https://reviews.apache.org/r/33339/#comment150703> please refactor all this code out into its own helper method. same below for WAITING src/examples/java/TestPersistentVolumeFramework.java (line 662) <https://reviews.apache.org/r/33339/#comment150704> you see? now anyone reading this code will be scratching their head trying to recall what contains which :) src/examples/java/TestPersistentVolumeFramework.java (line 669) <https://reviews.apache.org/r/33339/#comment150705> as everywhere else: `if (!errMsg.isEmpty())` but you can see that this is not a good pattern src/examples/java/TestPersistentVolumeFramework.java (line 678) <https://reviews.apache.org/r/33339/#comment150706> can you add a comment as to why you are doing this here? are you creating a new file on the slave? why? (I mean, add this to the comment, don't explain it just to me here, but to all future readers of this code) src/examples/java/TestPersistentVolumeFramework.java (lines 701 - 703) <https://reviews.apache.org/r/33339/#comment150709> I'm almost sure (but can't verify right now) that you can create this list with something like: ``` List<Operation> tmpOperations = Lists.newArrayList(getCreateOperation(volume), getLaunchOperation(task)); ``` or something similar. At the very least, take advantage of Java 7: ``` List<Operation> tmpOperations = new ArrayList<>(2); ``` src/examples/java/TestPersistentVolumeFramework.java (line 750) <https://reviews.apache.org/r/33339/#comment150710> unnecessary break also, I think you should just bail here? (I'd rather throw, but you can just return) src/examples/java/TestPersistentVolumeFramework.java (line 754) <https://reviews.apache.org/r/33339/#comment150711> use `diamond pattern` of Java 7 src/examples/java/TestPersistentVolumeFramework.java (line 769) <https://reviews.apache.org/r/33339/#comment150712> is there anything you should do here? if not, can you please add a comment as to why you ignore this? also, the log is pretty much irrelevant without a bit more info, if at all? src/examples/java/TestPersistentVolumeFramework.java (line 860) <https://reviews.apache.org/r/33339/#comment150713> factor this out into its own class src/examples/java/TestPersistentVolumeFramework.java (line 878) <https://reviews.apache.org/r/33339/#comment150715> s/slave/serverId use Javadoc instead of // comments - this way they show up in the docs src/examples/java/TestPersistentVolumeFramework.java (lines 889 - 901) <https://reviews.apache.org/r/33339/#comment150716> use javadoc also - is default (package) visibility what you want here? src/examples/java/TestPersistentVolumeFramework.java (line 912) <https://reviews.apache.org/r/33339/#comment150718> I think I already commented on my strong aversion to DIY classes. Apache has already a Flags class - please re-use that implementation: simplifies your code; less bugs to worry about; makes life easier for folks who can rely on documentation/examples in an established library. No need to re-invent the wheel :) src/examples/java/TestPersistentVolumeFramework.java (lines 1065 - 1066) <https://reviews.apache.org/r/33339/#comment150719> you really don't need these. If you do decide to keep them, please place them at the top of the class, where all CONSTANTS are expected to be. src/examples/java/TestPersistentVolumeFramework.java (line 1068) <https://reviews.apache.org/r/33339/#comment150720> please place your `main()` in a `Main` class and make sure we can run this via a simple: ``` java -jar PersistenceExampleFramework.jar Main ``` (or, even better, add the necessary flags in the pom.xml so that a MANIFEST is added as required). src/examples/java/TestPersistentVolumeFramework.java (lines 1099 - 1103) <https://reviews.apache.org/r/33339/#comment150722> ``` int status = driver.run() == Status.DRIVER_STOPPED ? EXIT_SUCCESS : EXIT_FAILURE; ``` src/examples/java/TestPersistentVolumeFramework.java (line 1105) <https://reviews.apache.org/r/33339/#comment150721> unnecessary; use ``` return status; ``` src/tests/examples_tests.cpp (line 43) <https://reviews.apache.org/r/33339/#comment150723> Can we please NOT add yet another test framework to the unit tests? already to run `make check` takes forever... src/tests/java_persistent_volume_framework_test.sh (lines 5 - 15) <https://reviews.apache.org/r/33339/#comment150726> Even Bash deserves some respect :) Please follow https://google-styleguide.googlecode.com/svn/trunk/shell.xml ``` HAS_ENV=$(env | grep "MESOS_SOURCE_DIR") if [[ -z ${HAS_ENV} ]]; then echo "..." exit 1 fi ``` same for BUILD_DIR - Marco Massenzio On June 21, 2015, 9:57 a.m., haosdent huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33339/ > ----------------------------------------------------------- > > (Updated June 21, 2015, 9:57 a.m.) > > > Review request for mesos, Adam B, Jie Yu, and Marco Massenzio. > > > 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 563e9c529444b3e980db6d04173f0d016a737c74 > src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 > src/examples/java/TestPersistentVolumeFramework.java PRE-CREATION > src/examples/java/test-persistent-volume-framework.in PRE-CREATION > src/tests/examples_tests.cpp 2ff6e7a449cc5037f9a3c8d6938855c35e389cca > src/tests/java_persistent_volume_framework_test.sh PRE-CREATION > > Diff: https://reviews.apache.org/r/33339/diff/ > > > Testing > ------- > > make check > > > Thanks, > > haosdent huang > >
