You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrei Budnik <ab...@mesosphere.com> on 2018/05/29 14:20:52 UTC
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/
-----------------------------------------------------------
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/1/
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
>
>
Re: Review Request 67354: Removed `os::getenv()` calls from
`MesosExecutorDriver`.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
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 Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67354/#review204120
-----------------------------------------------------------
include/mesos/executor.hpp
Lines 219-220 (patched)
<https://reviews.apache.org/r/67354/#comment286508>
Instead of copying the comment from above, please explain how this c-tor is differet from the one above. Maybe you can also add a note for the comment above that it can be unsafe to use it due to getenv calls.
src/exec/exec.cpp
Lines 636-638 (patched)
<https://reviews.apache.org/r/67354/#comment286509>
really? : )
- 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/1/
>
>
> Testing
> -------
>
> internal CI
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 67354: Removed `os::getenv()` calls from
`MesosExecutorDriver`.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On May 31, 2018, 6:24 p.m., Ilya Pronin wrote:
> > src/exec/exec.cpp
> > Lines 643-646 (original), 652-655 (patched)
> > <https://reviews.apache.org/r/67354/diff/1/?file=2031567#file2031567line652>
> >
> > We don't change executor logging flags in our tests, do we? It might be worth calling out in a comment that this constructor still loads those flags from environment variables.
I think ideally we should pass environment into `Flags.load()` here and entirely avoid reading environment if this c-tor is used.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67354/#review204131
-----------------------------------------------------------
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/1/
>
>
> Testing
> -------
>
> internal CI
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 67354: Removed `os::getenv()` calls from
`MesosExecutorDriver`.
Posted by Ilya Pronin <ip...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67354/#review204131
-----------------------------------------------------------
Looks good to me! Just one comment about logging flags.
src/exec/exec.cpp
Lines 643-646 (original), 652-655 (patched)
<https://reviews.apache.org/r/67354/#comment286521>
We don't change executor logging flags in our tests, do we? It might be worth calling out in a comment that this constructor still loads those flags from environment variables.
- Ilya Pronin
On May 29, 2018, 7:20 a.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67354/
> -----------------------------------------------------------
>
> (Updated May 29, 2018, 7:20 a.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/1/
>
>
> Testing
> -------
>
> internal CI
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 67354: Removed `os::getenv()` calls from
`MesosExecutorDriver`.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67354/#review204835
-----------------------------------------------------------
Fix it, then Ship it!
I'll fix the outstanding issues and commit shortly.
src/exec/exec.cpp
Line 646 (original), 654-655 (patched)
<https://reviews.apache.org/r/67354/#comment287578>
Add a TODO MESOS-9001.
- 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/3/
>
>
> Testing
> -------
>
> internal CI
>
>
> Thanks,
>
> Andrei Budnik
>
>