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



src/hdfs/hdfs.hpp (line 214)
<https://reviews.apache.org/r/39595/#comment163721>

    Please rebase and take this into account:
    
    https://issues.apache.org/jira/browse/MESOS-3602
    
    We want to also allow paths that start with hdfs://.



src/hdfs/hdfs.hpp (line 228)
<https://reviews.apache.org/r/39595/#comment163723>

    Does this output include the string which is in bad format? To be sure, I'd 
include it.



src/hdfs/hdfs.hpp (line 230)
<https://reviews.apache.org/r/39595/#comment163724>

    Here we could also print out.get() for extra diagnostics.



src/slave/containerizer/fetcher.hpp (line 156)
<https://reviews.apache.org/r/39595/#comment163725>

    Nice try changing the parameter conventions here. :-) But that's 
inconsistent with the rest of the code base, should be a separate ticket, which 
would lead to some discussion, first, to be sure. Please stick with the current 
style for now.



src/slave/containerizer/fetcher.hpp (line 164)
<https://reviews.apache.org/r/39595/#comment163726>

    We just don't use std::move this way anywhere in the code base AFAIK. We 
could start arguing about it right here, but if necessary let's better make it 
a separate style ticket and let's discuss there.



src/slave/containerizer/fetcher.cpp (line 247)
<https://reviews.apache.org/r/39595/#comment163729>

    No comment necessary above this line when changing this to SizeAndMTime, 
which matches the function name below exactly.



src/slave/containerizer/fetcher.cpp (line 254)
<https://reviews.apache.org/r/39595/#comment163730>

    Why no longer static?
    When you bring back static, you can also bring back the old param alignment.



src/slave/containerizer/fetcher.cpp (line 276)
<https://reviews.apache.org/r/39595/#comment163732>

    An advanced compiler will figure out that this is a std::move just by 
itself (using escape analysis). No need to clutter the code with extra hints. 
In case current C+= compilers weren't this good yet, let's endure some minimal 
memory and runtime cost here for now :-) 
    
    Readability first! std::move only when it is contributing something 
significant.



src/slave/containerizer/fetcher.cpp (line 281)
<https://reviews.apache.org/r/39595/#comment163734>

    This makes me believe the helper type SizeAndMTime should have been 
introduced in net.hpp already.



src/slave/containerizer/fetcher.cpp (line 320)
<https://reviews.apache.org/r/39595/#comment163735>

    no need to std:move IMHO



src/slave/containerizer/fetcher.cpp (line 399)
<https://reviews.apache.org/r/39595/#comment163740>

    Since this is missing, we are now unconditionally relying on the URI being 
reachable. But why not use the cache entry in case we learn nothing new about 
the URI, because it is unreachable?
    
    This also makes me think that we may want to make checking mtime optional?



src/slave/containerizer/fetcher.cpp (line 400)
<https://reviews.apache.org/r/39595/#comment163737>

    This does not need to be a closure and can be taken outside the fetch() 
method, since it does not capture anything.



src/slave/containerizer/fetcher.cpp (line 401)
<https://reviews.apache.org/r/39595/#comment163736>

    inconsistent naming



src/slave/containerizer/fetcher.cpp (line 408)
<https://reviews.apache.org/r/39595/#comment163742>

    Option<long>(spaceTime.mtime.isSome() ? ...



src/slave/containerizer/fetcher.cpp (line 421)
<https://reviews.apache.org/r/39595/#comment163743>

    Better use this value directly below. The aliasing only creates unnecessary 
intellectual load in the code.



src/slave/containerizer/fetcher.cpp (line 422)
<https://reviews.apache.org/r/39595/#comment163744>

    Either use this directly before it is used or do not create an alias at 
all. I vote for not aliasing since the value is only used twice.



src/slave/containerizer/fetcher.cpp (line 424)
<https://reviews.apache.org/r/39595/#comment163745>

    Redundant comment. s/some/a if you wanna keep it, since we should have 
precisely one cached entry for this URI.



src/slave/containerizer/fetcher.cpp (line 427)
<https://reviews.apache.org/r/39595/#comment163747>

    Why not pass down the shared_ptr like everywhere else?



src/slave/containerizer/fetcher.cpp (line 434)
<https://reviews.apache.org/r/39595/#comment163749>

    The first half of this sentence is redundant. The following statement 
already says this.
    
    The second half is helpful, but could be clearer if writing this BELOW the 
statement.
    
    "Falling through to downloading the URI again."



src/slave/containerizer/fetcher.cpp (line 439)
<https://reviews.apache.org/r/39595/#comment163748>

    Redundant comment. The next line says that already.



src/slave/containerizer/fetcher.cpp 
<https://reviews.apache.org/r/39595/#comment163728>

    What is gained by no longer checking this?


- Bernd Mathiske


On Oct. 23, 2015, 8:05 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39595/
> -----------------------------------------------------------
> 
> (Updated Oct. 23, 2015, 8:05 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Jan Schlicht, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3785
>     https://issues.apache.org/jira/browse/MESOS-3785
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Also added function to query mtime of HDFS resource.
> 
> 
> Diffs
> -----
> 
>   src/hdfs/hdfs.hpp 18f17231b92b84d0b0e4e15837d0e44ce8758cdf 
>   src/slave/containerizer/fetcher.hpp 
> c7518a36f6344841880dbb11bfce603fd2791fc0 
>   src/slave/containerizer/fetcher.cpp 
> e0d02d5f8f4f6e930a2ae6abe365548af6d1b01f 
>   src/tests/fetcher_cache_tests.cpp 0b3245105b4c1efae54f0bc34f672290819a6f0b 
> 
> Diff: https://reviews.apache.org/r/39595/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Reply via email to