mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <xuj...@apple.com>
Subject Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.
Date Thu, 02 Feb 2017 00:53:24 GMT


> On Feb. 1, 2017, 3:51 p.m., James Peach wrote:
> >

Could you directly reply to the comment in the future? It's pretty hard to follow with this
format...


> On Feb. 1, 2017, 3:51 p.m., James Peach wrote:
> > src/tests/containerizer/rootfs.hpp, line 50
> > <https://reviews.apache.org/r/53791/diff/8/?file=1589111#file1589111line50>
> >
> >     `copy()` would normally take a const reference to something and return a copy
of that. Since this is not what we are doing, best not to use that name.

Alright.


> On Feb. 1, 2017, 3:51 p.m., James Peach wrote:
> > src/tests/containerizer/rootfs.cpp, line 90
> > <https://reviews.apache.org/r/53791/diff/8/?file=1589112#file1589112line90>
> >
> >     What are you asking for here? This can fail for a number of reasons and `errno`
describes them. Squashing that to a fixed string would be misleading.

I am saying we know "This can fail for a number of reasons and errno describes them" because
we are breaking the abstraction here. Perhaps my arugment seems silly because `os::exists()`
has a 9-line implementation and we can look at the source. But if it's 900 lines or we can't
see the source the same principle still applies right?

`os::exists()` current exposes a `bool` and expects it to be used to interpret what's happened.
If we peek inside the implementation and write code based on the internals, it's likely to
break when the implementation changes.

Perhaps `bool os::exists()` is currently lacking in ability to reveal errors, should we then
change it to `Try<bool> exists()`? If that's more suitable as a TODO or we are not sure,
I think we should just leave it as it is right now. There are hundreds of other uses in the
codebase which may be susceptible to conditions like "the path does exist but we are not permitted
to access it" so maybe it's worth a JIRA to fix them anyways? (Of course that shouldn't block
our change here)


- Jiang Yan


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


On Feb. 1, 2017, 3:51 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> -----------------------------------------------------------
> 
> (Updated Feb. 1, 2017, 3:51 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
>     https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
> 
> 
> Diffs
> -----
> 
>   src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138 
>   src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e 
> 
> Diff: https://reviews.apache.org/r/53791/diff/
> 
> 
> Testing
> -------
> 
> sudo make check (Fedora 24)
> 
> 
> Thanks,
> 
> James Peach
> 
>


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