----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33339/#review95473 -----------------------------------------------------------
src/examples/java/TestPersistentVolumeFramework.java (line 34) <https://reviews.apache.org/r/33339/#comment150454> nit: An example... also: it's persistent *volumes* (plural) src/examples/java/TestPersistentVolumeFramework.java (line 38) <https://reviews.apache.org/r/33339/#comment150456> IMO it should not have the `Test` moniker in its name: it is usually reserved for junit.TestCase-derived classes that contain unit tests. As it's an "Example Framework" something like "SimplePersistentVolumeFramework" or "Example..." would describe better the intent. src/examples/java/TestPersistentVolumeFramework.java (line 41) <https://reviews.apache.org/r/33339/#comment150450> nit: `provides` also, you can lose the space before the ending period (also in the line below) src/examples/java/TestPersistentVolumeFramework.java (line 43) <https://reviews.apache.org/r/33339/#comment150452> add an empty line, please - the generated Javadoc won't wrap and will look not so pretty. Maybe you may want to wrap the filename in a `{@code}` so it renders as monospace. Finally, I'd suggest you lose the leading ./ and the trailing space before the period. src/examples/java/TestPersistentVolumeFramework.java (line 58) <https://reviews.apache.org/r/33339/#comment150457> s/use/uses This is useful information :) Please put it in the Javadoc and also make sure you mention in the @return clause src/examples/java/TestPersistentVolumeFramework.java (line 63) <https://reviews.apache.org/r/33339/#comment150467> This would be confusing to someone reading the comment (expecting some kind of `Error` class) and then seeing the method signature (which returns a `String`). I also find a `validate()` method returning an empty string to signal a valid resource completely unintuitive (and certainly, you should take advantage of the @return clause to at least mention that to your users). A much better pattern would be: (a) return a bool (`true` is valid, `false` otherwise); OR (b) return void, and `throw new ResourceParseException` with the reason as the message (and you could even go sophisticated by making ResourceParseException extend ParseException); OR (c) if your religion is of the no-exception kind ;-) you may instead implement an Error class which has a few enums, or a static `OK` instance and takes a `String` with a static constructor method. So many possibilities... src/examples/java/TestPersistentVolumeFramework.java (line 115) <https://reviews.apache.org/r/33339/#comment150470> with the caveat of the above comments, this should be anyway changed to: ``` if (!errMsg.isEmpty()) { ... ``` src/examples/java/TestPersistentVolumeFramework.java (line 126) <https://reviews.apache.org/r/33339/#comment150479> I really think it would be useful to enumerate the cases in which resources cannot be added (maybe with a list of <li> items?) ``` <li>if their reservation statuses don't match; <li>if only one has disk components; etc. ``` Remember: this is an "example" frameworks: many many folks who are *not* experts will come look at it to learn! src/examples/java/TestPersistentVolumeFramework.java (line 127) <https://reviews.apache.org/r/33339/#comment150471> s/addable/composable or "cannot be added together." src/examples/java/TestPersistentVolumeFramework.java (line 133) <https://reviews.apache.org/r/33339/#comment150490> s/addable/canAdd IMO all these methods could be non-static and be instead instance method: ``` public boolean canAdd(Resource other) ``` src/examples/java/TestPersistentVolumeFramework.java (line 146) <https://reviews.apache.org/r/33339/#comment150480> right - here for example: you could add a comment to explain why their being equal does not allow them to be added (I genuinely don't know! in fact, is that right?) src/examples/java/TestPersistentVolumeFramework.java (line 164) <https://reviews.apache.org/r/33339/#comment150482> didn't you just check this on L141? src/examples/java/TestPersistentVolumeFramework.java (line 173) <https://reviews.apache.org/r/33339/#comment150485> nit: s/subtractable/cannot be subtracted also: please use correct javadoc/HTML: ``` (insert blank line) <p><strong>NOTE:</strong> Set subtraction... ``` all the examples you enclose in quotes, IMO render better if you use {@literal} instead. src/examples/java/TestPersistentVolumeFramework.java (lines 185 - 189) <https://reviews.apache.org/r/33339/#comment150488> how about adding an `isCompatible(Resource other)` method to the `Resource` class? it's already the third time I see this code... DRY :) src/examples/java/TestPersistentVolumeFramework.java (lines 210 - 211) <https://reviews.apache.org/r/33339/#comment150489> Useful information! Drop the NOTE: and add to Javadoc src/examples/java/TestPersistentVolumeFramework.java (line 222) <https://reviews.apache.org/r/33339/#comment150493> take advantage of the @return clause ``` @return the compound resource, resulting from adding {@code left} and {@code right} together ``` even better, as I mentioned above (and same below): ``` /** * ... * @return a new Resource object, resulting from adding * this one to {@code other} */ public Resource add(Resource other) { if (!canAdd(other)) { throw new ResourceOperationException( String.format("Cannot add %s to %s", this, other)); } ... ``` (you would obviously need to implement a `toString()` for the Resource class for the above to work). src/examples/java/TestPersistentVolumeFramework.java (line 247) <https://reviews.apache.org/r/33339/#comment150502> s/a/an s/exist/existing s/resource list/list of resources and this comment is really too vague... Are you adding each resource in the list to that one, or the other way round? are you mutating either? both? none? (I mean, I can see what you do in the code :) but my point is your javadoc reader may not - and, in fact, shouldn't need to). Also, IMO, this should be a non-static and non-mutating operation on `resources` and return a new List instead. ``` public List<Resource> addTo(List<Resource> resources) { List<Resource> result = new ArrayList<>(resources.size()); boolean added = false; if (!isEmpty() && validate(this)) { for (Resource resource : resources) { if (!added && canAdd(resource)) { result.add(add(resource)); added = true; } else { result.add(resource); } } if (!added) { result.add(this); } } return result; } ``` Given the likely size of the Resources list (low tens? hundreds?) the memory/performance impact is likely to be minimal, but you gain the advantage of the functional, immutable programming style. src/examples/java/TestPersistentVolumeFramework.java (line 295) <https://reviews.apache.org/r/33339/#comment150504> nit: s/a exist/an existing (and everywhere else too) src/examples/java/TestPersistentVolumeFramework.java (line 300) <https://reviews.apache.org/r/33339/#comment150505> please modify this method in exactly the same way I described above for `add` src/examples/java/TestPersistentVolumeFramework.java (line 326) <https://reviews.apache.org/r/33339/#comment150506> the method's name, compound with its being static, and thus needing a left/right values, and finally the params' names - makes it mighty confusing for the reader to figure out whath contains what :) it really is NOT clear how you would use this method and its semantics; please provide a better description and an example (or two). IMO, this would be nicer if it were called `contains(List<> container, List<> contained)` but even better. Also, you **are mutating** the `left` list? you should mention this in the javadoc (also, why?) and probably rename the method. src/examples/java/TestPersistentVolumeFramework.java (line 339) <https://reviews.apache.org/r/33339/#comment150507> please update the description to actually state that you are checking that `contained` is at least contained in one of the `resources` and modify the args' names: IMO - this should be non static: ``` public boolean contained(List<Resource> resources) { for (Resource resource : resources) { if (resource.contains(this)) { return true; } } return false; } ``` and also change the `contains()` below to being non-static and checking that `this` contains `that`: ``` public boolean contains(Resource that) ``` src/examples/java/TestPersistentVolumeFramework.java (line 367) <https://reviews.apache.org/r/33339/#comment150508> see other comment too. make non-static and call `right` `that` instead. src/examples/java/TestPersistentVolumeFramework.java (line 375) <https://reviews.apache.org/r/33339/#comment150509> // Only Scalar type is currently supported. src/examples/java/TestPersistentVolumeFramework.java (line 384) <https://reviews.apache.org/r/33339/#comment150510> same comment as above for `validate` - do not use an empty string to signal a valid operation, that is just non-intuitive src/examples/java/TestPersistentVolumeFramework.java (line 399) <https://reviews.apache.org/r/33339/#comment150511> s/not used/is not currently used src/examples/java/TestPersistentVolumeFramework.java (line 404) <https://reviews.apache.org/r/33339/#comment150512> !errMsg.isEmpty() (but see my other comments too) src/examples/java/TestPersistentVolumeFramework.java (lines 468 - 472) <https://reviews.apache.org/r/33339/#comment150513> copy & paste of javadoc is almost as bad as copy & paste of code ;-) also, please here, folks will be wondering whether you apply **each** operation to **every** resource? what? do all succeed, or only a few? if one falls, do the others get rolled back? (I mean, I can see what goes on by looking at the code, but what about people just reading javadoc?) does it even make sense? src/examples/java/TestPersistentVolumeFramework.java (line 495) <https://reviews.apache.org/r/33339/#comment150514> can you please elaborate in the javadoc what this does in reality? src/examples/java/TestPersistentVolumeFramework.java (lines 501 - 503) <https://reviews.apache.org/r/33339/#comment150515> argh! hard-coded magic numbers alert!!! :D Please use constants - even better have them in a properties file - or something. and why do you only get 10% of a CPU??? (I mean, I know why, I just wish it were explained somewhere) Finally, having the units explicitly declared would be awesome for people not familiar with Mesos default units: int mem_mb or int disk_gb or whatever makes sense Again, sorry it's taken so long to get round to doing this review and soooo many thanks for doing this! I've only got halfway through, I'll try my best to do more in the next few days, less craziness (here's to hoping, anyway!) I notice the one file runs for >1,000 lines - the Resource class is probably worth having in its own .java file, and probably Flags too - maybe you can refactor further other parts too. In general, I like Java class files to only rarely exceed the 300-400 lines - bigger than that, it usually signals design choices that are sub-optimal in separating concerns. As I mentioned, it's great that you're doing this: as someone who wants to learn more about persistence framework, I'm looking forward to having this committed and being able to also hack around with it :) Maybe, we may also get a blog entry out of it, as we expose persistent volumes to a wider public and show folks how to use them in a Java framework. - 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 > >
