You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rukletsov <ru...@gmail.com> on 2018/06/13 11:31:32 UTC
Re: Review Request 67354: Removed `os::getenv()` calls from
`MesosExecutorDriver`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67354/#review204681
-----------------------------------------------------------
include/mesos/executor.hpp
Lines 220-223 (patched)
<https://reviews.apache.org/r/67354/#comment287291>
Maybe add this comment to the previous constructor saying that the other one is preferable? I think that way it will be easier for users to figure out they use a not favoured c-tor : )
src/exec/exec.cpp
Line 646 (original), 654 (patched)
<https://reviews.apache.org/r/67354/#comment287292>
I doubt this does what you want it to do. At the end, `FlagsBase` calls `extract()` on the prefix, which in turn calls `os::environment()`. Please either mention here that env is loaded under the hood (as Ilya suggested before), or fix the flags to not load the environment at all.
Maybe doing the latter should be part of a bigger change where we make `FlagsBase` instances never load from the environment (but provide a static function to do so). I will be happy to shepherd this work and chain it immediately after this change so it does not fall through the cracks.
- Alexander Rukletsov
On May 29, 2018, 2:20 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67354/
> -----------------------------------------------------------
>
> (Updated May 29, 2018, 2:20 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, haosdent huang, Ilya Pronin, and James Peach.
>
>
> Bugs: MESOS-3475
> https://issues.apache.org/jira/browse/MESOS-3475
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds overloaded constructor for `MesosExecutorDriver` that
> accepts `environment` parameter and stores it in the class variable.
> This new constructor is needed to get rid of `os::getenv()` calls,
> so that `MesosExecutorDriver` can be used in tests that require
> thread safety.
>
>
> Diffs
> -----
>
> include/mesos/executor.hpp d14c0369f6731100d27092142b56f108f8881003
> src/exec/exec.cpp 65a671d7ce83a51087d290ba039d18deba6313c2
>
>
> Diff: https://reviews.apache.org/r/67354/diff/2/
>
>
> Testing
> -------
>
> internal CI
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 67354: Removed `os::getenv()` calls from
`MesosExecutorDriver`.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On June 13, 2018, 11:31 a.m., Alexander Rukletsov wrote:
> > include/mesos/executor.hpp
> > Lines 220-223 (patched)
> > <https://reviews.apache.org/r/67354/diff/2/?file=2039584#file2039584line220>
> >
> > Maybe add this comment to the previous constructor saying that the other one is preferable? I think that way it will be easier for users to figure out they use a not favoured c-tor : )
Updated this and the following patches.
> On June 13, 2018, 11:31 a.m., Alexander Rukletsov wrote:
> > src/exec/exec.cpp
> > Line 646 (original), 654 (patched)
> > <https://reviews.apache.org/r/67354/diff/2/?file=2039585#file2039585line654>
> >
> > I doubt this does what you want it to do. At the end, `FlagsBase` calls `extract()` on the prefix, which in turn calls `os::environment()`. Please either mention here that env is loaded under the hood (as Ilya suggested before), or fix the flags to not load the environment at all.
> >
> > Maybe doing the latter should be part of a bigger change where we make `FlagsBase` instances never load from the environment (but provide a static function to do so). I will be happy to shepherd this work and chain it immediately after this change so it does not fall through the cracks.
We can filter environment vars ourselves and then call `FlagBase::load()` without specifying `prefix` argument to prevent it from reading global environment.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67354/#review204681
-----------------------------------------------------------
On May 29, 2018, 2:20 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67354/
> -----------------------------------------------------------
>
> (Updated May 29, 2018, 2:20 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, haosdent huang, Ilya Pronin, and James Peach.
>
>
> Bugs: MESOS-3475
> https://issues.apache.org/jira/browse/MESOS-3475
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds overloaded constructor for `MesosExecutorDriver` that
> accepts `environment` parameter and stores it in the class variable.
> This new constructor is needed to get rid of `os::getenv()` calls,
> so that `MesosExecutorDriver` can be used in tests that require
> thread safety.
>
>
> Diffs
> -----
>
> include/mesos/executor.hpp d14c0369f6731100d27092142b56f108f8881003
> src/exec/exec.cpp 65a671d7ce83a51087d290ba039d18deba6313c2
>
>
> Diff: https://reviews.apache.org/r/67354/diff/3/
>
>
> Testing
> -------
>
> internal CI
>
>
> Thanks,
>
> Andrei Budnik
>
>