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

Reply via email to