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

Ship it!


I like this approach a lot. Maybe some more comments could be useful for others 
to quickly grasp the reasonings behind less obvious things?


src/tests/hdfs_tests.cpp (lines 47 - 48)
<https://reviews.apache.org/r/41384/#comment170180>

    This confused me initially as it seems this does not really apply.
    The tests serialize bash scripts into this file which emulates the hadoop 
client's logic while operating on the local filesystem.



src/tests/hdfs_tests.cpp (line 64)
<https://reviews.apache.org/r/41384/#comment170182>

    Missing test heading comments in this case actually seems to be fine to me, 
given the easy to grasp nature and the expected repetition when insisting on 
doing so for every single one of these.



src/tests/hdfs_tests.cpp (line 69)
<https://reviews.apache.org/r/41384/#comment170184>

    Shall we add a comment on that we are emulating the version call as that 
one is excersized by the HDFS factory, testing the client?



src/tests/hdfs_tests.cpp (line 72)
<https://reviews.apache.org/r/41384/#comment170183>

    Would it make sense to somehow point to the expected hdfs-client argument 
syntax for the `exists` call (and the others as well)? In other words, why are 
we using the fourth argument here?
    
    Its `strings::format("%s fs -test -e '%s'", hadoop, absolutePath(path));` 
that explains this magic. Should we be verbose about that in some comments here 
and for the other calls?


- Till Toenshoff


On Dec. 15, 2015, 1:27 a.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41384/
> -----------------------------------------------------------
> 
> (Updated Dec. 15, 2015, 1:27 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-3951
>     https://issues.apache.org/jira/browse/MESOS-3951
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added tests for HDFS client.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 9762f8567e32d70b8df2d694a1fef5c692fc730e 
>   src/tests/hdfs_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41384/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Reply via email to