mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benjamin Bannier" <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 39595: Took mtime into account in the fetcher cache.
Date Thu, 05 Nov 2015 15:45:18 GMT


> On Nov. 5, 2015, 1:29 p.m., Bernd Mathiske wrote:
> > src/hdfs/hdfs.hpp, line 214
> > <https://reviews.apache.org/r/39595/diff/1/?file=1104940#file1104940line214>
> >
> >     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://.

Done. Note that this requires making `HDFS::absolutePath` `const` (which it should have been
all along).


> On Nov. 5, 2015, 1:29 p.m., Bernd Mathiske wrote:
> > src/hdfs/hdfs.hpp, line 228
> > <https://reviews.apache.org/r/39595/diff/1/?file=1104940#file1104940line228>
> >
> >     Does this output include the string which is in bad format? To be sure, I'd
include it.

I checked, and the faulty string is already included.


> On Nov. 5, 2015, 1:29 p.m., Bernd Mathiske wrote:
> > src/hdfs/hdfs.hpp, line 230
> > <https://reviews.apache.org/r/39595/diff/1/?file=1104940#file1104940line230>
> >
> >     Here we could also print out.get() for extra diagnostics.

Done.


> On Nov. 5, 2015, 1:29 p.m., Bernd Mathiske wrote:
> > src/slave/containerizer/fetcher.cpp, line 427
> > <https://reviews.apache.org/r/39595/diff/1/?file=1104942#file1104942line427>
> >
> >     Why not pass down the shared_ptr like everywhere else?

Since `shouldReuseCachedEntry` only examines an `entry` and e.g. doesn't need to extend its
lifetime there's no reason to pass on a `shared_ptr` (the copy would also needlessly work
on the contained atomic counter).


> On Nov. 5, 2015, 1:29 p.m., Bernd Mathiske wrote:
> > src/slave/containerizer/fetcher.cpp, line 254
> > <https://reviews.apache.org/r/39595/diff/1/?file=1104942#file1104942line254>
> >
> >     Why no longer static?
> >     When you bring back static, you can also bring back the old param alignment.

I wanted to enforce internal linkage for the added `SizeAndMtime` which for types would happen
by wrapping them in an anonymous namespace. Having a type in an anonymous namespace immediately
followed by its only user `fetchSize` which also has internal linkage, but enforced by `static`,
seemed wildly inconsistent to me.


> On Nov. 5, 2015, 1:29 p.m., Bernd Mathiske wrote:
> > src/slave/containerizer/fetcher.hpp, line 156
> > <https://reviews.apache.org/r/39595/diff/1/?file=1104941#file1104941line156>
> >
> >     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.

OK, rolled back since to intrusive.


> On Nov. 5, 2015, 1:29 p.m., Bernd Mathiske wrote:
> > src/slave/containerizer/fetcher.hpp, line 164
> > <https://reviews.apache.org/r/39595/diff/1/?file=1104941#file1104941line164>
> >
> >     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.

OK, not needed anymore after rolling back to constructor signature. Note that *we already
do `move` like that* in e.g. stout and libprocess.


> On Nov. 5, 2015, 1:29 p.m., Bernd Mathiske wrote:
> > src/slave/containerizer/fetcher.cpp, line 276
> > <https://reviews.apache.org/r/39595/diff/1/?file=1104942#file1104942line276>
> >
> >     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.

AFAIK C++ compilers in contrast to some Java compiler do not usually perform escape analysis.
They could perform (N)RVO, but that wouldn't apply here.

Since `Try` has among other things a `std::string message` performing a copy here would likely
incur a sloow heap allocation which we do not need (checked with trunk clang `-O2`). I wouldn't
agree that the few `std::move` clutter the code that much.


> On Nov. 5, 2015, 1:29 p.m., Bernd Mathiske wrote:
> > src/slave/containerizer/fetcher.cpp, line 320
> > <https://reviews.apache.org/r/39595/diff/1/?file=1104942#file1104942line320>
> >
> >     no need to std:move IMHO

See comment Re:fetcher.cpp:276.


> On Nov. 5, 2015, 1:29 p.m., Bernd Mathiske wrote:
> > src/slave/containerizer/fetcher.cpp, line 992
> > <https://reviews.apache.org/r/39595/diff/1/?file=1104942#file1104942line992>
> >
> >     What is gained by no longer checking this?

The semantics of that function have changed: instead of requiring the exact same entry (which
would now contain also the `mtime`), we now only require the same key which already unique
identifies entries. Since `FetcherProcess::Cache::contains` was used only when looking up
cached entry in the now modified part there was no reason to keep the old implementation which
wouldn't do what we need.


- Benjamin


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


On Nov. 5, 2015, 3:45 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39595/
> -----------------------------------------------------------
> 
> (Updated Nov. 5, 2015, 3:45 p.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 42c150186f2ce676407e4e00e84bd7e38063d9ba 
>   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