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

src/examples/java/TestPersistentVolumeFramework.java (lines 535 - 537)

    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)

    why `8`? what does this 'magic number' represent?
    is this something that should be a CONSTANT or a user/configuration 
    please also add a comment as to what is going on here.

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


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

    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 = 
      switch (type) {
        case Operation.Type.LAUNCH:
      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)

    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)

    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 
    This is just Java good practice and will set a good example.

src/examples/java/TestPersistentVolumeFramework.java (lines 647 - 651)

    this is going to be VERY verbose - replace with a log.debug(...)

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

    use `offers.size()` to dimension your new List (unless you expect it to 

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

    please refactor all this code out into its own helper method.
    same below for WAITING

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

    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)

    as everywhere else: `if (!errMsg.isEmpty())`
    but you can see that this is not a good pattern

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

    can you add a comment as to why you are doing this here? are you creating a 
new file on the slave?
    (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)

    I'm almost sure (but can't verify right now) that you can create this list 
with something like:
    List<Operation> tmpOperations = 
    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)

    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)

    use `diamond pattern` of Java 7

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

    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)

    factor this out into its own class

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

    use Javadoc instead of // comments - this way they show up in the docs

src/examples/java/TestPersistentVolumeFramework.java (lines 889 - 901)

    use javadoc
    also - is default (package) visibility what you want here?

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

    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)

    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)

    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)

    int status = driver.run() == Status.DRIVER_STOPPED ? EXIT_SUCCESS : 

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

    unnecessary; use
    return status;

src/tests/examples_tests.cpp (line 43)

    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)

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

Reply via email to