mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Till Toenshoff" <toensh...@me.com>
Subject Re: Review Request 41384: Added tests for HDFS client.
Date Tue, 15 Dec 2015 02:44:18 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message