You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2018/07/18 01:19:56 UTC
Review Request 67958: Added LibprocessTest for easily configuring the
library for a test.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67958/
-----------------------------------------------------------
Review request for mesos and Benjamin Mahler.
Repository: mesos
Description
-------
There was not a generic way for configuring and then reinitializing
the libprocess library in a test and the LibprocessTest fixture
provides this functionality.
Diffs
-----
3rdparty/libprocess/include/process/gtest.hpp fd4de8ab7c79940519b2e890a9b0b615ca9ca292
Diff: https://reviews.apache.org/r/67958/diff/1/
Testing
-------
make check
Thanks,
Benjamin Hindman
Re: Review Request 67958: Added LibprocessTest for easily configuring
the library for a test.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Nov. 6, 2018, 5:58 a.m., Benjamin Mahler wrote:
> > Ah, unfortunately, the use of `os::setenv` is going to break our parallel test runner :(
> >
> > We could have reinitialize take a flags object?
>
> Benjamin Bannier wrote:
> Not sure why this would be an issue for parallel test execution as e.g., the in-tree parallel test runner executes shards as separate processes (as also intended by gtest).
>
> I'd be more worried about concurrently executing actors reading from these globals while we mutate them. We have seen similar constructs in tests cause hard failures before. Similar issue below, and with `os::unsetenv`. Explicitly passing flags to `reinitialize` could allow us to pass this information down to the point where no actors are running anymore and we can safely set them.
As I was lying in bed afterwards I realized it doesn't conflict with the parallel test runner, and that I had mixed that up in my mind with the tests that concurrently touched getenv/setenv/unsetenv :)
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67958/#review210342
-----------------------------------------------------------
On July 18, 2018, 1:19 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67958/
> -----------------------------------------------------------
>
> (Updated July 18, 2018, 1:19 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> There was not a generic way for configuring and then reinitializing
> the libprocess library in a test and the LibprocessTest fixture
> provides this functionality.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/gtest.hpp fd4de8ab7c79940519b2e890a9b0b615ca9ca292
>
>
> Diff: https://reviews.apache.org/r/67958/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 67958: Added LibprocessTest for easily configuring
the library for a test.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Nov. 6, 2018, 6:58 a.m., Benjamin Mahler wrote:
> > Ah, unfortunately, the use of `os::setenv` is going to break our parallel test runner :(
> >
> > We could have reinitialize take a flags object?
Not sure why this would be an issue for parallel test execution as e.g., the in-tree parallel test runner executes shards as separate processes (as also intended by gtest).
I'd be more worried about concurrently executing actors reading from these globals while we mutate them. We have seen similar constructs in tests cause hard failures before. Similar issue below, and with `os::unsetenv`. Explicitly passing flags to `reinitialize` could allow us to pass this information down to the point where no actors are running anymore and we can safely set them.
> On Nov. 6, 2018, 6:58 a.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/include/process/gtest.hpp
> > Lines 62 (patched)
> > <https://reviews.apache.org/r/67958/diff/1/?file=2061359#file2061359line62>
> >
> > This prevents running tests in parallel :(
> >
> > Is there any way to avoid this?
See my comment above.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67958/#review210342
-----------------------------------------------------------
On July 18, 2018, 3:19 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67958/
> -----------------------------------------------------------
>
> (Updated July 18, 2018, 3:19 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> There was not a generic way for configuring and then reinitializing
> the libprocess library in a test and the LibprocessTest fixture
> provides this functionality.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/gtest.hpp fd4de8ab7c79940519b2e890a9b0b615ca9ca292
>
>
> Diff: https://reviews.apache.org/r/67958/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>
Re: Review Request 67958: Added LibprocessTest for easily configuring
the library for a test.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67958/#review210342
-----------------------------------------------------------
Ah, unfortunately, the use of `os::setenv` is going to break our parallel test runner :(
We could have reinitialize take a flags object?
3rdparty/libprocess/include/process/gtest.hpp
Lines 38-44 (patched)
<https://reviews.apache.org/r/67958/#comment295001>
Add an extra newline on top and bottom of this?
3rdparty/libprocess/include/process/gtest.hpp
Lines 38-40 (patched)
<https://reviews.apache.org/r/67958/#comment295002>
Is this best framed as a forward declaration? It seems to be better described as us exposing the declaration of the internal function since we need it for this? I'm left wondering after reading this whether it's exposed in some header or we're just surfacing it from it's .cpp file.
3rdparty/libprocess/include/process/gtest.hpp
Lines 52-71 (patched)
<https://reviews.apache.org/r/67958/#comment295004>
It looks like this doesn't handle being called twice (teardown then re-initializes back to the 2nd last call to re-initialize, rather than the original state).
We could fix that or crash if it's called twice?
3rdparty/libprocess/include/process/gtest.hpp
Lines 62 (patched)
<https://reviews.apache.org/r/67958/#comment295003>
This prevents running tests in parallel :(
Is there any way to avoid this?
- Benjamin Mahler
On July 18, 2018, 1:19 a.m., Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67958/
> -----------------------------------------------------------
>
> (Updated July 18, 2018, 1:19 a.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> There was not a generic way for configuring and then reinitializing
> the libprocess library in a test and the LibprocessTest fixture
> provides this functionality.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/gtest.hpp fd4de8ab7c79940519b2e890a9b0b615ca9ca292
>
>
> Diff: https://reviews.apache.org/r/67958/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Hindman
>
>