You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Marco Massenzio <ma...@mesosphere.io> on 2015/06/02 01:32:41 UTC
Re: Review Request 33339: Add a Java example framework to test
persistent volumes.
-----------------------------------------------------------
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
>
>
Re: Review Request 33339: Add a Java example framework to test
persistent volumes.
Posted by haosdent huang <ha...@gmail.com>.
> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > src/examples/java/TestPersistentVolumeFramework.java, lines 529-542
> > <https://reviews.apache.org/r/33339/diff/10/?file=941652#file941652line529>
> >
> > 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/33339/#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/33339/
> -----------------------------------------------------------
>
> (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/33339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 33339: Add a Java example framework to test
persistent volumes.
Posted by haosdent huang <ha...@gmail.com>.
> 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/33339/#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/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
>
>
Re: Review Request 33339: Add a Java example framework to test
persistent volumes.
Posted by haosdent huang <ha...@gmail.com>.
> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > src/examples/java/TestPersistentVolumeFramework.java, line 239
> > <https://reviews.apache.org/r/33339/diff/10/?file=941652#file941652line239>
> >
> > 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
> > <https://reviews.apache.org/r/33339/diff/10/?file=941652#file941652line301>
> >
> > 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
> > <https://reviews.apache.org/r/33339/diff/10/?file=941652#file941652line310>
> >
> > 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
> > <https://reviews.apache.org/r/33339/diff/10/?file=941652#file941652line344>
> >
> > 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
> > <https://reviews.apache.org/r/33339/diff/10/?file=941652#file941652line443>
> >
> > 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
> > <https://reviews.apache.org/r/33339/diff/10/?file=941652#file941652line529>
> >
> > 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/33339/#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/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
>
>
Re: Review Request 33339: Add a Java example framework to test
persistent volumes.
Posted by Marco Massenzio <ma...@mesosphere.io>.
> 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
> > <https://reviews.apache.org/r/33339/diff/10/?file=941652#file941652line29>
> >
> > 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
> > <https://reviews.apache.org/r/33339/diff/10/?file=941652#file941652line110>
> >
> > 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
> > <https://reviews.apache.org/r/33339/diff/10/?file=941652#file941652line344>
> >
> > 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
> > <https://reviews.apache.org/r/33339/diff/10/?file=941652#file941652line443>
> >
> > 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
What do you mean? the output would be the exact same?
> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > src/examples/java/TestPersistentVolumeFramework.java, lines 529-542
> > <https://reviews.apache.org/r/33339/diff/10/?file=941652#file941652line529>
> >
> > 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.
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?
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33339/#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/33339/
> -----------------------------------------------------------
>
> (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/33339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 33339: Add a Java example framework to test
persistent volumes.
Posted by Jie Yu <yu...@gmail.com>.
> 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/33339/#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/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
>
>
Re: Review Request 33339: Add a Java example framework to test
persistent volumes.
Posted by haosdent huang <ha...@gmail.com>.
> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > src/examples/java/TestPersistentVolumeFramework.java, line 29
> > <https://reviews.apache.org/r/33339/diff/10/?file=941652#file941652line29>
> >
> > 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/33339/#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/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
>
>
Re: Review Request 33339: Add a Java example framework to test
persistent volumes.
Posted by Jie Yu <yu...@gmail.com>.
> 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/33339/#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/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
>
>
Re: Review Request 33339: Add a Java example framework to test
persistent volumes.
Posted by haosdent huang <ha...@gmail.com>.
> 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/33339/#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/33339/
> -----------------------------------------------------------
>
> (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/33339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 33339: Add a Java example framework to test
persistent volumes.
Posted by Marco Massenzio <ma...@mesosphere.io>.
> 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/33339/#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/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
>
>
Re: Review Request 33339: Add a Java example framework to test
persistent volumes.
Posted by haosdent huang <ha...@gmail.com>.
> 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/33339/#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/33339/
> -----------------------------------------------------------
>
> (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/33339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 33339: Add a Java example framework to test
persistent volumes.
Posted by Adam B <ad...@mesosphere.io>.
> 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
> > <https://reviews.apache.org/r/33339/diff/10/?file=941652#file941652line529>
> >
> > 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
> > <https://reviews.apache.org/r/33339/diff/10/?file=941652#file941652line443>
> >
> > 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 344-352
> > <https://reviews.apache.org/r/33339/diff/10/?file=941652#file941652line344>
> >
> > 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
>
> Marco Massenzio wrote:
> And, maybe, that's A Good Thing :)
More importantly, the command value doesn't match:
"test -f volume/ persisted" (yours, extra space)
"test -f volume/persisted" (original)
> On June 1, 2015, 4:32 p.m., Marco Massenzio wrote:
> > src/examples/java/TestPersistentVolumeFramework.java, line 29
> > <https://reviews.apache.org/r/33339/diff/10/?file=941652#file941652line29>
> >
> > 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?
>
> Marco Massenzio wrote:
> My preference would be to add documentation and be a "good guy" to whoever will follow in your footstep and appreciate the comments ;)
+1 to documentation for any public classes/methods you can reasonably comment on (I know you're not an expert yet). To decrease your documentation burden, let's pretend that the scheduler methods you @Override have awesome documentation elsewhere, and you only need to add comments specific to your implementation. You could also make more of these methods private, which is generally a good idea, not just for avoiding comments. ;)
> On June 1, 2015, 4:32 p.m., Marco Massenzio wrote:
> > src/examples/java/TestPersistentVolumeFramework.java, line 239
> > <https://reviews.apache.org/r/33339/diff/10/?file=941652#file941652line239>
> >
> > 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`
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.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33339/#review86113
-----------------------------------------------------------
On June 7, 2015, 1:10 a.m., haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33339/
> -----------------------------------------------------------
>
> (Updated June 7, 2015, 1: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/33339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 33339: Add a Java example framework to test
persistent volumes.
Posted by haosdent huang <ha...@gmail.com>.
> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > src/examples/java/TestPersistentVolumeFramework.java, line 239
> > <https://reviews.apache.org/r/33339/diff/10/?file=941652#file941652line239>
> >
> > 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/33339/#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/33339/
> -----------------------------------------------------------
>
> (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/33339/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 33339: Add a Java example framework to test
persistent volumes.
Posted by haosdent huang <ha...@gmail.com>.
> On June 1, 2015, 11:32 p.m., Marco Massenzio wrote:
> > src/examples/java/TestPersistentVolumeFramework.java, line 110
> > <https://reviews.apache.org/r/33339/diff/10/?file=941652#file941652line110>
> >
> > 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
> > <https://reviews.apache.org/r/33339/diff/10/?file=941652#file941652line112>
> >
> > 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/33339/#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/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
>
>