mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bernd Mathiske" <be...@mesosphere.io>
Subject Re: Review Request 39595: Took mtime into account in the fetcher cache.
Date Thu, 05 Nov 2015 13:29:47 GMT

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


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