Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[Marco Massenzio]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/#review95611
---



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

please also add a comment as to what is going on here.



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


s/by/for



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


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

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 
grow?)



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


I'm almost sure (but can't verify right now) that you can create this list 
with something like:

```
List tmpOperations = 
Lists.newArrayList(getCreateOperation(volume),
getLaunchOperation(task));
```

or something similar.
At the very least, take advantage of Java 7:
```
List tmpOperations = new ArrayList<>(2);
```



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


unnecessary break
also, I think you should just bail here?
(I

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[haosdent huang]

On Aug. 14, 2015, 10:28 p.m., haosdent huang wrote:
> > Again, sorry it's taken so long to get round to doing this review and s 
> > 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.

Thank you for your work! Let me update it.


- haosdent


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/#review95473
---


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/9/
> ---
> 
> (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/9/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[Marco Massenzio]

> On Aug. 14, 2015, 10:28 p.m., Marco Massenzio wrote:
> >

[mmm turns out that it matters WHICH boxes you put your general comments in :) 
- copied here, as they make sense *before* the nitpicking that follows]

Again, sorry it's taken so long to get round to doing this review and s 
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


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/#review95473
---


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/9/
> ---
> 
> (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/9/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[Marco Massenzio]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/#review95473
---



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



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


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



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

Finally, I'd suggest you lose the leading ./ and the trailing space before 
the period.



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


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)


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)


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  items?)
```
if their reservation statuses don't match;
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)


s/addable/composable
or
"cannot be added together."



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


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)


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)

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

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/#review88697
---


Patch looks great!

Reviews applied: [9]

All tests passed.

- Mesos ReviewBot


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/9/
> ---
> 
> (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/9/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[haosdent huang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/
---

(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 (updated)
-

  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/9/diff/


Testing
---

make check


Thanks,

haosdent huang

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[haosdent huang]

> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > src/examples/java/TestPersistentVolumeFramework.java, lines 529-542
> > 
> >
> > Please consider using Apache Commons CLI instead: 
> > https://commons.apache.org/proper/commons-cli/
> 
> haosdent huang wrote:
> Yes, but it would add a new dependence to Maven. Is it acceptable? Other 
> exist java examples also simple handle the args.
> 
> Marco Massenzio wrote:
> I am not sure what the "official policy" is here - but this is a widely 
> used library (and not a major dependency either) so I think we should be fine 
> with it.
> Anyone got any opinion on this?
> 
> Adam B wrote:
> Anything from Apache Commons Proper should be fine. We're all Apache here.

@marco, @adam-mesos . I still don't like add log4j or commons-cli to this 
simple examples, because 
* Add external dependences to example may let user hard to run. Dependences may 
also conflict with user exist dependences.
* If we want to add external jar dependences, we need add a pom.xml or a gradle 
file. Currently our java examples even don't have packages. Change a example to 
maven project maybe affect more code. If we decide to change examples to a 
maven project or gradle project, I need should open a new review to change them.

So I just a Flags java implementation to handle command line options in this 
patch, I think it should be more readable now.


- haosdent


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/#review86113
---


On June 21, 2015, 9:42 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9/
> ---
> 
> (Updated June 21, 2015, 9:42 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/9/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[haosdent huang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/
---

(Updated June 21, 2015, 9:42 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 (updated)
-

  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/9/diff/


Testing
---

make check


Thanks,

haosdent huang

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/#review88691
---


Patch looks great!

Reviews applied: [9]

All tests passed.

- Mesos ReviewBot


On June 21, 2015, 5:37 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9/
> ---
> 
> (Updated June 21, 2015, 5:37 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/9/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[haosdent huang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/
---

(Updated June 21, 2015, 5:37 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 (updated)
-

  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/9/diff/


Testing
---

make check


Thanks,

haosdent huang

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[haosdent huang]

> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > src/examples/java/TestPersistentVolumeFramework.java, line 239
> > 
> >
> > 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.
> 
> haosdent huang wrote:
> Hmm, name it to TestPersistentVolumeScheduler is follow other exist 
> examples. Like `TestScheduler` in `TestFramework.java`
> 
> Adam B wrote:
> At least a few of the other example frameworks are run as part of the 
> unit test suite, under ExamplesTest: 
> https://github.com/apache/mesos/blob/0.22.1/src/tests/examples_tests.cpp#L36
> See also: 
> https://github.com/apache/mesos/blob/0.22.1/src/examples/java/test-framework.in
> I'm not asking you to add TestPersistentVolumeFramework to ExamplesTests 
> (although it would be awesome!). I'm just explaining why some of the 
> frameworks are named TestXxx; others are just copycats.

Hmm, it could run as a unit test.


- haosdent


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/#review86113
---


On June 21, 2015, 5:17 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9/
> ---
> 
> (Updated June 21, 2015, 5:17 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/9/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[haosdent huang]

> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > 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.
> 
> Jie Yu wrote:
> Sorry, this is my fault. I really don't have cycle for this at this 
> moment. Marco, mind sherperding this patch?
> 
> Jie Yu wrote:
> The logic of this patch should match that in 
> src/examples/persistent_volume_framework.cpp
> 
> Marco Massenzio wrote:
> Jie - I would be absolutely happy to do that, with one minor hitch: I'm 
> not a committer :)
> Happy to help out wherever I can, though!
> 
> haosdent huang wrote:
> Thank you very much for your review, let me update it. @marco
> 
> haosdent huang wrote:
> @marco Thank you for your review again. But some logic are follow the 
> exists Java examples and persistent_volume_framework.cpp . I am not sure 
> change it are correct or not. Anyway, I think I need to reflect this patch to 
> make it more clear. After I reflect it, I would @you and looking forward your 
> review again. Thanks a lot.
> 
> Marco Massenzio wrote:
> I understand that - and therein lies the problem :)
> As you proved, code in 'examples' is widely "used as a template" (aka, 
> copied) so it would be great if we could set an... example, by providing 
> great code there.
> 
> Translating Java almost verbatim from C++ actually results in 
> non-idiomatic constructs that either (a) look weird to Java practitioners or 
> (b) engender bad practices (that are then propagated by the "oh, but that's 
> how it's done in the other examples" - sounds familiar? :)
> 
> My suggestions (and I stress it here, they are "suggestions" - if 
> committers with more Java experience than I are happy with your code, then 
> that's just fine) are to provide some great examples, that improve on 
> existing code.
> 
> Thanks for doing this!
> 
> Adam B wrote:
> Thanks haosdent for coding this up, and thanks Marco for the initial 
> review. As a Java/C++ polyglot, I'll volunteer to Shepherd this patch, if Jie 
> will help verify that it properly tests his feature.
> Unfortunately, it seems the authors of the original Java examples/tests 
> were not hardened Java developers, and their poor style habits have been 
> copied into subsequent examples. I won't ask haosdent to solve all of those 
> issues, or else this patch would be indefinitely delayed. As long as it's no 
> worse than the other examples, and adequately demonstrates/tests the feature, 
> I'll let it through.

@marco, @adam-mesos I reflect the code, could you review it again? Thank you 
very much.


- haosdent


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/#review86113
---


On June 21, 2015, 5:17 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9/
> ---
> 
> (Updated June 21, 2015, 5:17 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/9/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[haosdent huang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/
---

(Updated June 21, 2015, 5:17 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 (updated)
-

  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/9/diff/


Testing
---

make check


Thanks,

haosdent huang

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[Adam B]

> On June 1, 2015, 4:32 p.m., Marco Massenzio wrote:
> > 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.
> 
> Jie Yu wrote:
> Sorry, this is my fault. I really don't have cycle for this at this 
> moment. Marco, mind sherperding this patch?
> 
> Jie Yu wrote:
> The logic of this patch should match that in 
> src/examples/persistent_volume_framework.cpp
> 
> Marco Massenzio wrote:
> Jie - I would be absolutely happy to do that, with one minor hitch: I'm 
> not a committer :)
> Happy to help out wherever I can, though!
> 
> haosdent huang wrote:
> Thank you very much for your review, let me update it. @marco
> 
> haosdent huang wrote:
> @marco Thank you for your review again. But some logic are follow the 
> exists Java examples and persistent_volume_framework.cpp . I am not sure 
> change it are correct or not. Anyway, I think I need to reflect this patch to 
> make it more clear. After I reflect it, I would @you and looking forward your 
> review again. Thanks a lot.
> 
> Marco Massenzio wrote:
> I understand that - and therein lies the problem :)
> As you proved, code in 'examples' is widely "used as a template" (aka, 
> copied) so it would be great if we could set an... example, by providing 
> great code there.
> 
> Translating Java almost verbatim from C++ actually results in 
> non-idiomatic constructs that either (a) look weird to Java practitioners or 
> (b) engender bad practices (that are then propagated by the "oh, but that's 
> how it's done in the other examples" - sounds familiar? :)
> 
> My suggestions (and I stress it here, they are "suggestions" - if 
> committers with more Java experience than I are happy with your code, then 
> that's just fine) are to provide some great examples, that improve on 
> existing code.
> 
> Thanks for doing this!

Thanks haosdent for coding this up, and thanks Marco for the initial review. As 
a Java/C++ polyglot, I'll volunteer to Shepherd this patch, if Jie will help 
verify that it properly tests his feature.
Unfortunately, it seems the authors of the original Java examples/tests were 
not hardened Java developers, and their poor style habits have been copied into 
subsequent examples. I won't ask haosdent to solve all of those issues, or else 
this patch would be indefinitely delayed. As long as it's no worse than the 
other examples, and adequately demonstrates/tests the feature, I'll let it 
through.


> On June 1, 2015, 4:32 p.m., Marco Massenzio wrote:
> > src/examples/java/TestPersistentVolumeFramework.java, lines 529-542
> > 
> >
> > Please consider using Apache Commons CLI instead: 
> > https://commons.apache.org/proper/commons-cli/
> 
> haosdent huang wrote:
> Yes, but it would add a new dependence to Maven. Is it acceptable? Other 
> exist java examples also simple handle the args.
> 
> Marco Massenzio wrote:
> I am not sure what the "official policy" is here - but this is a widely 
> used library (and not a major dependency either) so I think we should be fine 
> with it.
> Anyone got any opinion on this?

Anything from Apache Commons Proper should be fine. We're all Apache here.


> On June 1, 2015, 4:32 p.m., Marco Massenzio wrote:
> > src/examples/java/TestPersistentVolumeFramework.java, lines 443-444
> > 
> >
> > 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);
> > ```
> 
> haosdent huang wrote:
> Sure, but would not match
> 
> ```
> LOG(INFO) << "Received framework message from executor '" << executorId
>   << "' on slave " << slaveId << ": '" << data << "'";
> ```
> 
> in persistent_volume_framework.cpp
> 
> Marco Massenzio wrote:
> What do you mean? the output would be the exact same?

I agree with Marco that the output would be formatted exactly the same, and 
standard string concatenation is slow in Java, so most people use StringBuilder 
or even String.format. On the other hand, this test framework is not 
performance-critical, and none of the other Java examples/tests use 
StringBuilder/String.format. Fix it as many places as you like, but I won't 
hold up the patch for it.


> On June 1, 2015, 4:32 p.m., Marco Massenzio wrote:
> > src/examples/java/TestPersistentVolumeFramework.java, lines 3

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[Marco Massenzio]

> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > 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.
> 
> Jie Yu wrote:
> Sorry, this is my fault. I really don't have cycle for this at this 
> moment. Marco, mind sherperding this patch?
> 
> Jie Yu wrote:
> The logic of this patch should match that in 
> src/examples/persistent_volume_framework.cpp
> 
> Marco Massenzio wrote:
> Jie - I would be absolutely happy to do that, with one minor hitch: I'm 
> not a committer :)
> Happy to help out wherever I can, though!
> 
> haosdent huang wrote:
> Thank you very much for your review, let me update it. @marco
> 
> haosdent huang wrote:
> @marco Thank you for your review again. But some logic are follow the 
> exists Java examples and persistent_volume_framework.cpp . I am not sure 
> change it are correct or not. Anyway, I think I need to reflect this patch to 
> make it more clear. After I reflect it, I would @you and looking forward your 
> review again. Thanks a lot.

I understand that - and therein lies the problem :)
As you proved, code in 'examples' is widely "used as a template" (aka, copied) 
so it would be great if we could set an... example, by providing great code 
there.

Translating Java almost verbatim from C++ actually results in non-idiomatic 
constructs that either (a) look weird to Java practitioners or (b) engender bad 
practices (that are then propagated by the "oh, but that's how it's done in the 
other examples" - sounds familiar? :)

My suggestions (and I stress it here, they are "suggestions" - if committers 
with more Java experience than I are happy with your code, then that's just 
fine) are to provide some great examples, that improve on existing code.

Thanks for doing this!


> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > src/examples/java/TestPersistentVolumeFramework.java, line 29
> > 
> >
> > 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!
> 
> haosdent huang wrote:
> Acording to the exist java example framework 
> (TestExecutor.java/TestMultipleExecutorsFramework.java/...), don't have 
> javadocs here. Do we still need add javadocs here, or keep same as other 
> exist examples?

My preference would be to add documentation and be a "good guy" to whoever will 
follow in your footstep and appreciate the comments ;)


> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > src/examples/java/TestPersistentVolumeFramework.java, line 110
> > 
> >
> > here too
> 
> haosdent huang wrote:
> here could not remove because `j` is used in `remains.set(j, newRemain);`,

yes, I noticed that - I'm wondering whether there's a more Java-idiomatic way 
of achieving same.
Not a big deal, but definitely something worth considering.


> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > src/examples/java/TestPersistentVolumeFramework.java, lines 344-352
> > 
> >
> > this is almost identical code to L307-315: how about factoring out to a 
> > common `buildTaskInfo(String command)`?
> 
> haosdent huang wrote:
> Sure, but it would not match the code 
> 
> ```cpp
> TaskInfo task;
> task.set_name(shard.name);
> task.mutable_task_id()->set_value(UUID::random().toString());
> task.mutable_slave_id()->CopyFrom(offer.slave_id());
> task.mutable_resources()->CopyFrom(shard.resources);
> task.mutable_command()->set_value("test -f volume/persisted");
> ```
> 
> in persistent_volume_framework.cpp

And, maybe, that's A Good Thing :)


> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > src/examples/java/TestPersistentVolumeFramework.java, lines 443-444
> > 
> >
> > instead of concatenating string, either use a StringBuilder or, even 
> > better:
> > ```
> > String.format("Received framework message from executor '%s' on sl

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/#review86956
---


Patch looks great!

Reviews applied: [9]

All tests passed.

- Mesos ReviewBot


On June 7, 2015, 8:10 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9/
> ---
> 
> (Updated June 7, 2015, 8:10 a.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 cad7f0e92eacc86d37b3f578382946db8b466531 
>   src/Makefile.am ec7f41f2114d02d3d5a1434d9799f15fe9331917 
>   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/9/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[haosdent huang]

> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > 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.
> 
> Jie Yu wrote:
> Sorry, this is my fault. I really don't have cycle for this at this 
> moment. Marco, mind sherperding this patch?
> 
> Jie Yu wrote:
> The logic of this patch should match that in 
> src/examples/persistent_volume_framework.cpp
> 
> Marco Massenzio wrote:
> Jie - I would be absolutely happy to do that, with one minor hitch: I'm 
> not a committer :)
> Happy to help out wherever I can, though!
> 
> haosdent huang wrote:
> Thank you very much for your review, let me update it. @marco

@marco Thank you for your review again. But some logic are follow the exists 
Java examples and persistent_volume_framework.cpp . I am not sure change it are 
correct or not. Anyway, I think I need to reflect this patch to make it more 
clear. After I reflect it, I would @you and looking forward your review again. 
Thanks a lot.


- haosdent


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/#review86113
---


On June 7, 2015, 8:10 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9/
> ---
> 
> (Updated June 7, 2015, 8:10 a.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 cad7f0e92eacc86d37b3f578382946db8b466531 
>   src/Makefile.am ec7f41f2114d02d3d5a1434d9799f15fe9331917 
>   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/9/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[haosdent huang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/
---

(Updated June 7, 2015, 8:10 a.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 (updated)
-

  configure.ac cad7f0e92eacc86d37b3f578382946db8b466531 
  src/Makefile.am ec7f41f2114d02d3d5a1434d9799f15fe9331917 
  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/9/diff/


Testing
---

make check


Thanks,

haosdent huang

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[haosdent huang]

> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > src/examples/java/TestPersistentVolumeFramework.java, line 239
> > 
> >
> > 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.

Hmm, name it to TestPersistentVolumeScheduler is follow other exist examples. 
Like `TestScheduler` in `TestFramework.java`


> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > src/examples/java/TestPersistentVolumeFramework.java, lines 301-304
> > 
> >
> > 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

Use `System.out.println` here is also follow other exist Java examples.


> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > src/examples/java/TestPersistentVolumeFramework.java, line 310
> > 
> >
> > 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!)

Hmm, we don't need build here because Java protobuf api could pass a builder. 
`+ ""` should change to `toString` here.


> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > src/examples/java/TestPersistentVolumeFramework.java, lines 344-352
> > 
> >
> > this is almost identical code to L307-315: how about factoring out to a 
> > common `buildTaskInfo(String command)`?

Sure, but it would not match the code 

```cpp
TaskInfo task;
task.set_name(shard.name);
task.mutable_task_id()->set_value(UUID::random().toString());
task.mutable_slave_id()->CopyFrom(offer.slave_id());
task.mutable_resources()->CopyFrom(shard.resources);
task.mutable_command()->set_value("test -f volume/persisted");
```

in persistent_volume_framework.cpp


> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > src/examples/java/TestPersistentVolumeFramework.java, lines 443-444
> > 
> >
> > 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);
> > ```

Sure, but would not match

```
LOG(INFO) << "Received framework message from executor '" << executorId
  << "' on slave " << slaveId << ": '" << data << "'";
```

in persistent_volume_framework.cpp


> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > src/examples/java/TestPersistentVolumeFramework.java, lines 529-542
> > 
> >
> > Please consider using Apache Commons CLI instead: 
> > https://commons.apache.org/proper/commons-cli/

Yes, but it would add a new dependence to Maven. Is it acceptable? Other exist 
java examples also simple handle the args.


- haosdent


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/#review86113
---


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/9/
> ---
> 
> (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/9/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[haosdent huang]

> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > src/examples/java/TestPersistentVolumeFramework.java, line 110
> > 
> >
> > here too

here could not remove because `j` is used in `remains.set(j, newRemain);`,


> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > src/examples/java/TestPersistentVolumeFramework.java, lines 112-116
> > 
> >
> > 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.

In fact, this snippet is convert from src/common/resources.cpp. So let me think 
whether a better way to implement it.


- haosdent


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/#review86113
---


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/9/
> ---
> 
> (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/9/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[haosdent huang]

> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > src/examples/java/TestPersistentVolumeFramework.java, line 29
> > 
> >
> > 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!

Acording to the exist java example framework 
(TestExecutor.java/TestMultipleExecutorsFramework.java/...), don't have 
javadocs here. Do we still need add javadocs here, or keep same as other exist 
examples?


- haosdent


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/#review86113
---


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/9/
> ---
> 
> (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/9/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[haosdent huang]

> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > 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.
> 
> Jie Yu wrote:
> Sorry, this is my fault. I really don't have cycle for this at this 
> moment. Marco, mind sherperding this patch?
> 
> Jie Yu wrote:
> The logic of this patch should match that in 
> src/examples/persistent_volume_framework.cpp
> 
> Marco Massenzio wrote:
> Jie - I would be absolutely happy to do that, with one minor hitch: I'm 
> not a committer :)
> Happy to help out wherever I can, though!

Thank you very much for your review, let me update it. @marco


- haosdent


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/#review86113
---


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/9/
> ---
> 
> (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/9/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[Marco Massenzio]

> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > 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.
> 
> Jie Yu wrote:
> Sorry, this is my fault. I really don't have cycle for this at this 
> moment. Marco, mind sherperding this patch?
> 
> Jie Yu wrote:
> The logic of this patch should match that in 
> src/examples/persistent_volume_framework.cpp

Jie - I would be absolutely happy to do that, with one minor hitch: I'm not a 
committer :)
Happy to help out wherever I can, though!


- Marco


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/#review86113
---


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/9/
> ---
> 
> (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/9/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[Jie Yu]

> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > 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.
> 
> Jie Yu wrote:
> Sorry, this is my fault. I really don't have cycle for this at this 
> moment. Marco, mind sherperding this patch?

The logic of this patch should match that in 
src/examples/persistent_volume_framework.cpp


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/#review86113
---


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/9/
> ---
> 
> (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/9/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[Jie Yu]

> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > 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.

Sorry, this is my fault. I really don't have cycle for this at this moment. 
Marco, mind sherperding this patch?


- Jie


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/#review86113
---


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/9/
> ---
> 
> (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/9/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[Marco Massenzio]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/#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


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


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


please avoid this; use instead the `foreach` pattern:
```
for (Resource require : requires) {
...
```



src/examples/java/TestPersistentVolumeFramework.java


here too



src/examples/java/TestPersistentVolumeFramework.java


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


here too



src/examples/java/TestPersistentVolumeFramework.java


do you really need the `else-continue` here?



src/examples/java/TestPersistentVolumeFramework.java


a better name for `remain` would be something like `consumed` or `allocated`



src/examples/java/TestPersistentVolumeFramework.java


can we refactor this very complex comparison into its own (aptly  named) 
method?
(with some javadoc too :)



src/examples/java/TestPersistentVolumeFramework.java


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


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


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


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


shouldn't you `.build()` here?

also, a better pattern is to use `.toString()` (as opposed t

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[haosdent huang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/
---

(Updated April 26, 2015, 7:10 a.m.)


Review request for mesos and Jie Yu.


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/9/diff/


Testing (updated)
---

make check


Thanks,

haosdent huang

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/#review81595
---


Patch looks great!

Reviews applied: [9]

All tests passed.

- Mesos ReviewBot


On April 25, 2015, 6:07 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9/
> ---
> 
> (Updated April 25, 2015, 6:07 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> 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/9/diff/
> 
> 
> Testing
> ---
> 
> make test
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[haosdent huang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/
---

(Updated April 25, 2015, 6:07 p.m.)


Review request for mesos and Jie Yu.


Bugs: MESOS-2610
https://issues.apache.org/jira/browse/MESOS-2610


Repository: mesos


Description
---

Add a Java example framework to test persistent volumes.


Diffs (updated)
-

  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/9/diff/


Testing
---

make test


Thanks,

haosdent huang

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[haosdent huang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/
---

(Updated April 25, 2015, 6:05 p.m.)


Review request for mesos and Jie Yu.


Bugs: MESOS-2610
https://issues.apache.org/jira/browse/MESOS-2610


Repository: mesos


Description (updated)
---

Add a Java example framework to test persistent volumes.


Diffs (updated)
-

  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/9/diff/


Testing (updated)
---

make test


Thanks,

haosdent huang

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[haosdent huang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/
---

(Updated April 25, 2015, 1:25 p.m.)


Review request for mesos and Jie Yu.


Bugs: MESOS-2610
https://issues.apache.org/jira/browse/MESOS-2610


Repository: mesos


Description
---

Add a Java example framework to test persistent volumes.
This is a sample patch, should could not work now. I would continue update it 
and post test result.


Diffs (updated)
-

  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/9/diff/


Testing
---


Thanks,

haosdent huang

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[haosdent huang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/
---

(Updated April 25, 2015, 1:23 p.m.)


Review request for mesos and Jie Yu.


Bugs: MESOS-2610
https://issues.apache.org/jira/browse/MESOS-2610


Repository: mesos


Description
---

Add a Java example framework to test persistent volumes.
This is a sample patch, should could not work now. I would continue update it 
and post test result.


Diffs (updated)
-

  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/9/diff/


Testing
---


Thanks,

haosdent huang

Re: Review Request 33339: Add a Java example framework to test persistent volumes.

[haosdent huang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9/
---

(Updated April 25, 2015, 10:17 a.m.)


Review request for mesos and Jie Yu.


Bugs: MESOS-2610
https://issues.apache.org/jira/browse/MESOS-2610


Repository: mesos


Description
---

Add a Java example framework to test persistent volumes.
This is a sample patch, should could not work now. I would continue update it 
and post test result.


Diffs (updated)
-

  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/9/diff/


Testing
---


Thanks,

haosdent huang