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, 17 Aug 2015 19:27:30 GMT

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


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