Re: Review Request 39595: Took mtime into account in the fetcher cache.

[Benjamin Bannier]

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

(Updated Nov. 13, 2015, 12:16 p.m.)


Review request for mesos, Bernd Mathiske, Jan Schlicht, and Till Toenshoff.


Changes
---

Updated for changed typenames from RR-39595.


Bugs: MESOS-3785
https://issues.apache.org/jira/browse/MESOS-3785


Repository: mesos


Description
---

Also added function to query mtime of HDFS resource.


Diffs (updated)
-

  src/hdfs/hdfs.hpp 42c150186f2ce676407e4e00e84bd7e38063d9ba 
  src/slave/containerizer/fetcher.hpp c7518a36f6344841880dbb11bfce603fd2791fc0 
  src/slave/containerizer/fetcher.cpp 0b676f43e8529c6790824c8cf82e21a81d885ef1 
  src/tests/fetcher_cache_tests.cpp 7060a151f8812e9fef654419377991970ab8b961 

Diff: https://reviews.apache.org/r/39595/diff/


Testing
---

make check


Thanks,

Benjamin Bannier

Re: Review Request 39595: Took mtime into account in the fetcher cache.

[Jan Schlicht]

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

Ship it!



src/hdfs/hdfs.hpp (lines 205 - 210)


Indent with an extra space, like
```
/**
 * Foo
 * 
 * Bar to describe foo.
 */
```



src/hdfs/hdfs.hpp (line 206)


Each line of a doxygen comment should start with '*'.



src/slave/containerizer/fetcher.cpp (line 248)


Indent with 4 spaces.



src/slave/containerizer/fetcher.cpp (line 325)


Indent with 4 spaces.



src/slave/containerizer/fetcher.cpp (line 935)


Indent with 4 spaces.


- Jan Schlicht


On Nov. 10, 2015, 2:45 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39595/
> ---
> 
> (Updated Nov. 10, 2015, 2: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 
> 0b676f43e8529c6790824c8cf82e21a81d885ef1 
>   src/tests/fetcher_cache_tests.cpp 0b3245105b4c1efae54f0bc34f672290819a6f0b 
> 
> Diff: https://reviews.apache.org/r/39595/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 39595: Took mtime into account in the fetcher cache.

[Benjamin Bannier]

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

(Updated Nov. 10, 2015, 4:14 p.m.)


Review request for mesos, Bernd Mathiske, Jan Schlicht, and Till Toenshoff.


Changes
---

More fixes for trivial clang-format misformats (NFC).


Bugs: MESOS-3785
https://issues.apache.org/jira/browse/MESOS-3785


Repository: mesos


Description
---

Also added function to query mtime of HDFS resource.


Diffs (updated)
-

  src/hdfs/hdfs.hpp 42c150186f2ce676407e4e00e84bd7e38063d9ba 
  src/slave/containerizer/fetcher.hpp c7518a36f6344841880dbb11bfce603fd2791fc0 
  src/slave/containerizer/fetcher.cpp 0b676f43e8529c6790824c8cf82e21a81d885ef1 
  src/tests/fetcher_cache_tests.cpp 0b3245105b4c1efae54f0bc34f672290819a6f0b 

Diff: https://reviews.apache.org/r/39595/diff/


Testing
---

make check


Thanks,

Benjamin Bannier

Re: Review Request 39595: Took mtime into account in the fetcher cache.

[Benjamin Bannier]

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

(Updated Nov. 10, 2015, 4:19 p.m.)


Review request for mesos, Bernd Mathiske, Jan Schlicht, and Till Toenshoff.


Changes
---

[fixup] More fixes for trivial clang-format misformats (NFC).


Bugs: MESOS-3785
https://issues.apache.org/jira/browse/MESOS-3785


Repository: mesos


Description
---

Also added function to query mtime of HDFS resource.


Diffs (updated)
-

  src/hdfs/hdfs.hpp 42c150186f2ce676407e4e00e84bd7e38063d9ba 
  src/slave/containerizer/fetcher.hpp c7518a36f6344841880dbb11bfce603fd2791fc0 
  src/slave/containerizer/fetcher.cpp 0b676f43e8529c6790824c8cf82e21a81d885ef1 
  src/tests/fetcher_cache_tests.cpp 0b3245105b4c1efae54f0bc34f672290819a6f0b 

Diff: https://reviews.apache.org/r/39595/diff/


Testing
---

make check


Thanks,

Benjamin Bannier

Re: Review Request 39595: Took mtime into account in the fetcher cache.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39594, 39595]

All tests passed.

- Mesos ReviewBot


On Nov. 10, 2015, 1:45 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39595/
> ---
> 
> (Updated Nov. 10, 2015, 1: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 
> 0b676f43e8529c6790824c8cf82e21a81d885ef1 
>   src/tests/fetcher_cache_tests.cpp 0b3245105b4c1efae54f0bc34f672290819a6f0b 
> 
> Diff: https://reviews.apache.org/r/39595/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 39595: Took mtime into account in the fetcher cache.

[Jan Schlicht]

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



src/slave/containerizer/fetcher.hpp (line 252)


Not yours, but this function can probably be const.



src/slave/containerizer/fetcher.cpp (line 247)


Newline after "(" to reduce jaggedness.



src/slave/containerizer/fetcher.cpp (line 262)


Newline after this line please.



src/slave/containerizer/fetcher.cpp (line 934)


4 spaces instead of 2 spaces.



src/slave/containerizer/fetcher.cpp (line 972)


Function can be made const.


- Jan Schlicht


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

Re: Review Request 39595: Took mtime into account in the fetcher cache.

[Benjamin Bannier]

> On Nov. 9, 2015, 1:01 p.m., Till Toenshoff wrote:
> > src/hdfs/hdfs.hpp, lines 204-208
> > 
> >
> > The examples of our doxygen-styleguide suggest that we should be using 
> > C style comments. Same goes with the parameter description which should be 
> > starting with a capital letter.
> > 
> > ```
> >   /**
> >* Obtain path modification time.
> >*
> >* @param path Path to the resource to query.
> >* @return Returns the last modification time of the resource in 
> > seconds
> >*   since the POSIX epoch, or an Error if it cannot be retrieved.
> >*/
> > ```

Of course!


> On Nov. 9, 2015, 1:01 p.m., Till Toenshoff wrote:
> > src/slave/containerizer/fetcher.hpp, line 256
> > 
> >
> > Let's only use the key here...
> > 
> > ```
> > bool hasKey(const std::string& key);
> > ```

Done, also a `const` member fct now.


- Benjamin


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


On Nov. 10, 2015, 1:45 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39595/
> ---
> 
> (Updated Nov. 10, 2015, 1: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 
> 0b676f43e8529c6790824c8cf82e21a81d885ef1 
>   src/tests/fetcher_cache_tests.cpp 0b3245105b4c1efae54f0bc34f672290819a6f0b 
> 
> Diff: https://reviews.apache.org/r/39595/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 39595: Took mtime into account in the fetcher cache.

[Benjamin Bannier]

> On Nov. 10, 2015, 12:52 p.m., Jan Schlicht wrote:
> > src/slave/containerizer/fetcher.cpp, line 940
> > 
> >
> > 4 spaces instead of 2 spaces.

It appears that continuations for assignments are indented by 2 spaces, not by 
4. Please re-check your version of the C++ style guide.


> On Nov. 10, 2015, 12:52 p.m., Jan Schlicht wrote:
> > src/slave/containerizer/fetcher.cpp, line 262
> > 
> >
> > Newline after this line please.

Fair enough, added an extra newline.


- Benjamin


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


On Nov. 10, 2015, 1:45 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39595/
> ---
> 
> (Updated Nov. 10, 2015, 1: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 
> 0b676f43e8529c6790824c8cf82e21a81d885ef1 
>   src/tests/fetcher_cache_tests.cpp 0b3245105b4c1efae54f0bc34f672290819a6f0b 
> 
> Diff: https://reviews.apache.org/r/39595/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 39595: Took mtime into account in the fetcher cache.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [39594]

Failed command: ./support/apply-review.sh -n -r 39594

Error:
 2015-11-10 13:53:38 URL:https://reviews.apache.org/r/39594/diff/raw/ 
[3630/3630] -> "39594.patch" [1]
error: patch failed: 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp:85
error: 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp: patch does not 
apply
Failed to apply patch

- Mesos ReviewBot


On Nov. 10, 2015, 1:45 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39595/
> ---
> 
> (Updated Nov. 10, 2015, 1: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 
> 0b676f43e8529c6790824c8cf82e21a81d885ef1 
>   src/tests/fetcher_cache_tests.cpp 0b3245105b4c1efae54f0bc34f672290819a6f0b 
> 
> Diff: https://reviews.apache.org/r/39595/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 39595: Took mtime into account in the fetcher cache.

[Till Toenshoff]

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



src/hdfs/hdfs.hpp (lines 204 - 208)


The examples of our doxygen-styleguide suggest that we should be using C 
style comments. Same goes with the parameter description which should be 
starting with a capital letter.

```
  /**
   * Obtain path modification time.
   *
   * @param path Path to the resource to query.
   * @return Returns the last modification time of the resource in seconds
   *   since the POSIX epoch, or an Error if it cannot be retrieved.
   */
```



src/hdfs/hdfs.hpp (line 237)


Good catch.



src/slave/containerizer/fetcher.hpp (line 255)


Let's only use the key here...

```
bool hasKey(const std::string& key);
```



src/slave/containerizer/fetcher.cpp (line 318)


`if(sizeAndMtime.size.isError()) {` ?



src/slave/containerizer/fetcher.cpp (line 325)


This is not how we wrap ternary operator expressions. We use variable 
indentation...

```
const auto optionalMtime = Option(
sizeAndMtime.mtime.isSome() ? Option(sizeAndMtime.mtime.get())
: None());
```


- Till Toenshoff


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

Re: Review Request 39595: Took mtime into account in the fetcher cache.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39594, 39595]

All tests passed.

- Mesos ReviewBot


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

Re: Review Request 39595: Took mtime into account in the fetcher cache.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [39594]

Failed command: ./support/apply-review.sh -n -r 39594

Error:
 2015-11-07 00:41:56 URL:https://reviews.apache.org/r/39594/diff/raw/ 
[3198/3198] -> "39594.patch" [1]
error: patch failed: 
3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp:123
error: 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp: patch does not 
apply
Failed to apply patch

- Mesos ReviewBot


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

Re: Review Request 39595: Took mtime into account in the fetcher cache.

[Benjamin Bannier]

> On Nov. 5, 2015, 1:29 p.m., Bernd Mathiske wrote:
> > src/slave/containerizer/fetcher.cpp, line 254
> > 
> >
> > Why no longer static?
> > When you bring back static, you can also bring back the old param 
> > alignment.
> 
> Benjamin Bannier wrote:
> 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.

With `SizeAndMtime` gone from here I made the functions `static` again.


- Benjamin


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


On Nov. 6, 2015, 11:04 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39595/
> ---
> 
> (Updated Nov. 6, 2015, 11:04 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 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
> 
>

Re: Review Request 39595: Took mtime into account in the fetcher cache.

[Benjamin Bannier]

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

(Updated Nov. 6, 2015, 11:04 a.m.)


Review request for mesos, Bernd Mathiske, Jan Schlicht, and Till Toenshoff.


Changes
---

Moved `SizeAndMtime` to `net.hpp`.


Bugs: MESOS-3785
https://issues.apache.org/jira/browse/MESOS-3785


Repository: mesos


Description
---

Also added function to query mtime of HDFS resource.


Diffs (updated)
-

  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

Re: Review Request 39595: Took mtime into account in the fetcher cache.

[Benjamin Bannier]

> On Nov. 5, 2015, 1:29 p.m., Bernd Mathiske wrote:
> > src/slave/containerizer/fetcher.cpp, line 276
> > 
> >
> > 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.
> 
> Benjamin Bannier wrote:
> 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.

Remove all usages of `move` here and in RR-39594 since the consensus seems to 
be that it worsens readability beyond what we care about performance in this 
case.


- Benjamin


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


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

Re: Review Request 39595: Took mtime into account in the fetcher cache.

[Benjamin Bannier]

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

(Updated Nov. 6, 2015, 7:52 p.m.)


Review request for mesos, Bernd Mathiske, Jan Schlicht, and Till Toenshoff.


Changes
---

Do not `move` to improve readbility.


Bugs: MESOS-3785
https://issues.apache.org/jira/browse/MESOS-3785


Repository: mesos


Description
---

Also added function to query mtime of HDFS resource.


Diffs (updated)
-

  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

Re: Review Request 39595: Took mtime into account in the fetcher cache.

[Benjamin Bannier]

> On Nov. 5, 2015, 1:29 p.m., Bernd Mathiske wrote:
> > src/slave/containerizer/fetcher.cpp, line 399
> > 
> >
> > 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?

Good point. I changed the cache reuse policy to now always reuse existing cache 
entries in case the source URI cannot be queried, so that we do not need to 
fail anymore just because of a missing source.

I left checking of the mtime as is; the implementation already treats mtime as 
an optional value: since empty `Options` compare equal we would already not 
take it into account in case it is not available.


- Benjamin


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


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

Re: Review Request 39595: Took mtime into account in the fetcher cache.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39594, 39595]

All tests passed.

- Mesos ReviewBot


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

Re: Review Request 39595: Took mtime into account in the fetcher cache.

[Benjamin Bannier]

> On Nov. 5, 2015, 1:29 p.m., Bernd Mathiske wrote:
> > src/hdfs/hdfs.hpp, line 214
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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 

Re: Review Request 39595: Took mtime into account in the fetcher cache.

[Benjamin Bannier]

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


Changes
---

Rebased and incorporated most review comments.

Using `absolutePath` required to make `HDFS::absolutePath` `const`.
Still need to decide what to do e.g. about unavailable remotes.


Bugs: MESOS-3785
https://issues.apache.org/jira/browse/MESOS-3785


Repository: mesos


Description
---

Also added function to query mtime of HDFS resource.


Diffs (updated)
-

  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

Re: Review Request 39595: Took mtime into account in the fetcher cache.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39594, 39595]

All tests passed.

- Mesos ReviewBot


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

Review Request 39595: Took mtime into account in the fetcher cache.

[Benjamin Bannier]

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

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