mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marco Massenzio" <ma...@mesosphere.io>
Subject Re: Review Request 33339: Add a Java example framework to test persistent volumes.
Date Mon, 01 Jun 2015 23:32:41 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message