This is an automatically generated e-mail. To reply, visit:

src/examples/java/TestPersistentVolumeFramework.java (line 34)

    nit: An example...
    also: it's persistent *volumes* (plural)

src/examples/java/TestPersistentVolumeFramework.java (line 38)

    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 

src/examples/java/TestPersistentVolumeFramework.java (line 41)

    nit: `provides`
    also, you can lose the space before the ending period (also in the line 

src/examples/java/TestPersistentVolumeFramework.java (line 43)

    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 
    Finally, I'd suggest you lose the leading ./ and the trailing space before 
the period.

src/examples/java/TestPersistentVolumeFramework.java (line 58)

    This is useful information :)
    Please put it in the Javadoc and also make sure you mention in the @return 

src/examples/java/TestPersistentVolumeFramework.java (line 63)

    This would be confusing to someone reading the comment (expecting some kind 
of `Error` class) and then seeing the method signature (which returns a 
    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)

    with the caveat of the above comments, this should be anyway changed to:
    if (!errMsg.isEmpty()) {

src/examples/java/TestPersistentVolumeFramework.java (line 126)

    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;
    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)

    "cannot be added together."

src/examples/java/TestPersistentVolumeFramework.java (line 133)

    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)

    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)

    didn't you just check this on L141?

src/examples/java/TestPersistentVolumeFramework.java (line 173)

    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)

    how about adding an `isCompatible(Resource other)` method to the `Resource` 
    it's already the third time I see this code... DRY :)

src/examples/java/TestPersistentVolumeFramework.java (lines 210 - 211)

    Useful information!
    Drop the NOTE: and add to Javadoc

src/examples/java/TestPersistentVolumeFramework.java (line 222)

    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)

    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)) {
            added = true;
          } else {
        if (!added) {
      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)

    nit: s/a exist/an existing
    (and everywhere else too)

src/examples/java/TestPersistentVolumeFramework.java (line 300)

    please modify this method in exactly the same way I described above for 

src/examples/java/TestPersistentVolumeFramework.java (line 326)

    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)

    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)

    see other comment too.
    make non-static and call `right` `that` instead.

src/examples/java/TestPersistentVolumeFramework.java (line 375)

    // Only Scalar type is currently supported.

src/examples/java/TestPersistentVolumeFramework.java (line 384)

    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)

    s/not used/is not currently used

src/examples/java/TestPersistentVolumeFramework.java (line 404)

    (but see my other comments too)

src/examples/java/TestPersistentVolumeFramework.java (lines 468 - 472)

    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)

    can you please elaborate in the javadoc what this does in reality?

src/examples/java/TestPersistentVolumeFramework.java (lines 501 - 503)

    argh! hard-coded magic numbers alert!!! :D
    Please use constants - even better have them in a properties file - or 
    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

Reply via email to