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 Sekretenko <as...@mesosphere.io> on 2019/06/03 19:55:50 UTC
Re: Review Request 70671: Added a class for setting expectations on
master V1 API events in tests.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70671/
-----------------------------------------------------------
(Updated June 3, 2019, 7:55 p.m.)
Review request for mesos, Benno Evers, Benjamin Mahler, and Joseph Wu.
Changes
-------
Implemented workaround to avoid libprocess deadlock; fixed other remaining issues.
Bugs: MESOS-7258
https://issues.apache.org/jira/browse/MESOS-7258
Repository: mesos
Description
-------
This patch introduces a class with mock methods for subscribing to the
events of master's V1 streaming API and setting expectations for them.
Diffs (updated)
-----
src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f
src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
src/tests/master/mock_master_api_subscriber.hpp PRE-CREATION
src/tests/master/mock_master_api_subscriber.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/70671/diff/5/
Changes: https://reviews.apache.org/r/70671/diff/4-5/
Testing
-------
Thanks,
Andrei Sekretenko
Re: Review Request 70671: Added a class for setting expectations on
master V1 API events in tests.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
> On June 10, 2019, 3:08 a.m., Benjamin Mahler wrote:
> > Looks good, thanks!
> >
> > I will make some adjustments prior to committing, the biggest is to remove the need for the nullptr assingment and loop check, by using finalize() instead of shutdown(). Also will add a receiveLoop future that we discard when finalizing:
> >
> > ```
> > diff --git a/src/tests/master/mock_master_api_subscriber.cpp b/src/tests/master/mock_master_api_subscriber.cpp
> > index 8c49c1af5..a1cd53844 100644
> > --- a/src/tests/master/mock_master_api_subscriber.cpp
> > +++ b/src/tests/master/mock_master_api_subscriber.cpp
> > @@ -44,7 +44,7 @@ class MockMasterAPISubscriberProcess
> > {
> > public:
> > MockMasterAPISubscriberProcess(MockMasterAPISubscriber* subscriber_)
> > - : Process<MockMasterAPISubscriberProcess>(), subscriber(subscriber_){};
> > + : subscriber(subscriber_) {};
> >
> > Future<Nothing> subscribe(
> > const process::PID<Master>& pid, ContentType contentType)
> > @@ -64,15 +64,13 @@ public:
> > .then(defer(self(), &Self::_subscribe, lambda::_1, contentType));
> > }
> >
> > - Nothing shutdown()
> > +protected:
> > + void finalize() override
> > {
> > - subscriber = nullptr;
> > - return Nothing();
> > + receiveLoop.discard();
> > }
> >
> > private:
> > - MockMasterAPISubscriber* subscriber;
> > -
> > Future<Nothing> _subscribe(
> > const process::Future<process::http::Response>& response,
> > ContentType contentType)
> > @@ -106,7 +104,7 @@ private:
> > [](std::unique_ptr<Reader<Event>>& d) { return d->read(); },
> > std::move(reader));
> >
> > - process::loop(
> > + receiveLoop = process::loop(
> > self(),
> > std::move(decode),
> > [this](const Result<Event>& event) -> process::ControlFlow<Nothing> {
> > @@ -123,17 +121,21 @@ private:
> > LOG(INFO) << "Received " << event->type()
> > << " event from master streaming API";
> >
> > - if (subscriber != nullptr) {
> > - subscriber->handleEvent(event.get());
> > - }
> > -
> > + subscriber->handleEvent(event.get());
> > return process::Continue();
> > });
> >
> > LOG(INFO) << "Subscribed to master streaming API events";
> >
> > + receiveLoop.onAny([]() {
> > + LOG(INFO) << "Stopped master streaming API receive loop";
> > + });
> > +
> > return Nothing();
> > }
> > +
> > + MockMasterAPISubscriber* subscriber;
> > + Future<Nothing> receiveLoop;
> > };
> >
> >
> > @@ -175,12 +177,14 @@ MockMasterAPISubscriber::MockMasterAPISubscriber()
> >
> > MockMasterAPISubscriber::~MockMasterAPISubscriber()
> > {
> > - process::Future<Nothing> shutdown =
> > - dispatch(process, &MockMasterAPISubscriberProcess::shutdown);
> > -
> > - shutdown.get();
> > -
> > - terminate(process);
> > + process::terminate(process);
> > +
> > + // The process holds a pointer to this object, and so
> > + // we must ensure it won't access the pointer before
> > + // we exit the destructor.
> > + //
> > + // TODO(asekretenko): Figure out a way to avoid blocking.
> > + process::wait(process);
> > }
> >
> >
> >
> > ```
Thanks for cleaning this up!
There really were some leftovers of the workaround for the libprocess deadlock, I should have removed them myself.
However, I do not fully understand one of your changes: discarding the `receiveLoop` future.
While running the tests, I'm observing that:
- the onAny callback of `receiveLoop` is not called (at least typically)
- removing `receiveLoop.discard()` does not affect the tests (at least, the typical runs)
- if, before terminating this process, I perform a discard+await on this future, the await hangs. (If I understand the code of the `Reader` correctly, the reason behind the hang is that the promise which satisfies the future returned by `reader->read()` is never discarded.)
So I'm left puzzled - why is it needed to discard this future?... Is it some corner case which does not show up under typical conditions, or something else?
> On June 10, 2019, 3:08 a.m., Benjamin Mahler wrote:
> > src/tests/master/mock_master_api_subscriber.cpp
> > Lines 171-172 (patched)
> > <https://reviews.apache.org/r/70671/diff/6/?file=2148193#file2148193line171>
> >
> > Why do we need the pointer? We can use a PID?
> >
> > ```
> > process = spawn(new MockMasterAPISubscriberProcess(this), true);
> > ```
> >
> > That way, we also don't have the strangeness of holding a pointer to a Process that is managed (which I don't think is done anywhere in our code).
> >
> > Is it because `PID<MockMasterAPISubscriberProcess>` doesn't work with it forward declared?
You are right, there is no need to store the pointer. The follow-up patch: https://reviews.apache.org/r/70843/
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70671/#review215771
-----------------------------------------------------------
On June 7, 2019, 4:21 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70671/
> -----------------------------------------------------------
>
> (Updated June 7, 2019, 4:21 p.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, and Joseph Wu.
>
>
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch introduces a class with mock methods for subscribing to the
> events of master's V1 streaming API and setting expectations for them.
>
>
> Diffs
> -----
>
> src/Makefile.am 93b2606841745c74dc33713e6b1b2fd62a1c7e74
> src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
> src/tests/master/mock_master_api_subscriber.hpp PRE-CREATION
> src/tests/master/mock_master_api_subscriber.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/70671/diff/6/
>
>
> Testing
> -------
>
> `./bin/mesos-tests.sh --verbose --gtest_filter="*UpdateFramework*" --gtest_break_on_failure --gtest_repeat=1000` with new tests from https://reviews.apache.org/r/70534/
>
> `./bin/mesos-tests.sh --verbose --gtest_filter="*MasterAPITest*" --gtest_break_on_failure --gtest_repeat=1000` with refactored tests from https://reviews.apache.org/r/70756/
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70671: Added a class for setting expectations on
master V1 API events in tests.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70671/#review215771
-----------------------------------------------------------
Fix it, then Ship it!
Looks good, thanks!
I will make some adjustments prior to committing, the biggest is to remove the need for the nullptr assingment and loop check, by using finalize() instead of shutdown(). Also will add a receiveLoop future that we discard when finalizing:
```
diff --git a/src/tests/master/mock_master_api_subscriber.cpp b/src/tests/master/mock_master_api_subscriber.cpp
index 8c49c1af5..a1cd53844 100644
--- a/src/tests/master/mock_master_api_subscriber.cpp
+++ b/src/tests/master/mock_master_api_subscriber.cpp
@@ -44,7 +44,7 @@ class MockMasterAPISubscriberProcess
{
public:
MockMasterAPISubscriberProcess(MockMasterAPISubscriber* subscriber_)
- : Process<MockMasterAPISubscriberProcess>(), subscriber(subscriber_){};
+ : subscriber(subscriber_) {};
Future<Nothing> subscribe(
const process::PID<Master>& pid, ContentType contentType)
@@ -64,15 +64,13 @@ public:
.then(defer(self(), &Self::_subscribe, lambda::_1, contentType));
}
- Nothing shutdown()
+protected:
+ void finalize() override
{
- subscriber = nullptr;
- return Nothing();
+ receiveLoop.discard();
}
private:
- MockMasterAPISubscriber* subscriber;
-
Future<Nothing> _subscribe(
const process::Future<process::http::Response>& response,
ContentType contentType)
@@ -106,7 +104,7 @@ private:
[](std::unique_ptr<Reader<Event>>& d) { return d->read(); },
std::move(reader));
- process::loop(
+ receiveLoop = process::loop(
self(),
std::move(decode),
[this](const Result<Event>& event) -> process::ControlFlow<Nothing> {
@@ -123,17 +121,21 @@ private:
LOG(INFO) << "Received " << event->type()
<< " event from master streaming API";
- if (subscriber != nullptr) {
- subscriber->handleEvent(event.get());
- }
-
+ subscriber->handleEvent(event.get());
return process::Continue();
});
LOG(INFO) << "Subscribed to master streaming API events";
+ receiveLoop.onAny([]() {
+ LOG(INFO) << "Stopped master streaming API receive loop";
+ });
+
return Nothing();
}
+
+ MockMasterAPISubscriber* subscriber;
+ Future<Nothing> receiveLoop;
};
@@ -175,12 +177,14 @@ MockMasterAPISubscriber::MockMasterAPISubscriber()
MockMasterAPISubscriber::~MockMasterAPISubscriber()
{
- process::Future<Nothing> shutdown =
- dispatch(process, &MockMasterAPISubscriberProcess::shutdown);
-
- shutdown.get();
-
- terminate(process);
+ process::terminate(process);
+
+ // The process holds a pointer to this object, and so
+ // we must ensure it won't access the pointer before
+ // we exit the destructor.
+ //
+ // TODO(asekretenko): Figure out a way to avoid blocking.
+ process::wait(process);
}
```
src/tests/master/mock_master_api_subscriber.cpp
Lines 47 (patched)
<https://reviews.apache.org/r/70671/#comment302605>
Why was the explicit parent constructor call needed?
s/){}/) {}/
src/tests/master/mock_master_api_subscriber.cpp
Lines 67-71 (patched)
<https://reviews.apache.org/r/70671/#comment302606>
This is called "shutdown" but it doesn't shut anything down. The loop still runs when it encounters 'nullptr' and we don't discard the loop future, it would be good to stop the loop here and also have the loop break if it sees nullptr.
E.g.
```
// TODO(asekretenko): Ideally this function returns the receive
// loop future so that shutdown completes when the receive
// loop is stopped. That way, we also wouldn't need to assign
// `subscriber` to nullptr and check it in the loop.
receiveLoop.discard();
```
Also, this can just be done within `finalize()` and the caller can `process::wait()` for it?
Actually, if we take the `finalize()` approach, we don't need to assign to null pointer and check it.
src/tests/master/mock_master_api_subscriber.cpp
Lines 126-130 (patched)
<https://reviews.apache.org/r/70671/#comment302607>
Per the above comment, we should break when we encounter the nullptr?
```
if (subscriber == nullptr) {
return process::Break();
}
subscriber->handleEvent(event.get());
return process::Continue();
```
(Actually, we don't need this if we use finalize() instead of adding shutdown()).
src/tests/master/mock_master_api_subscriber.cpp
Lines 171-172 (patched)
<https://reviews.apache.org/r/70671/#comment302608>
Why do we need the pointer? We can use a PID?
```
process = spawn(new MockMasterAPISubscriberProcess(this), true);
```
That way, we also don't have the strangeness of holding a pointer to a Process that is managed (which I don't think is done anywhere in our code).
Is it because `PID<MockMasterAPISubscriberProcess>` doesn't work with it forward declared?
src/tests/master/mock_master_api_subscriber.cpp
Lines 176-184 (patched)
<https://reviews.apache.org/r/70671/#comment302604>
Generally, blocking (.get()) is banned as it can induce deadlock if all of the worker threads block, but in this case since we're passing a back pointer I don't see a way to avoid it.
A comment would be helpful to the reader about why this is being done:
```
// The process holds a pointer to this object, and so
// we must ensure it won't access the pointer before
// we exit the destructor.
//
// TODO(asekretenko): Figure out a way to avoid blocking.
```
- Benjamin Mahler
On June 7, 2019, 4:21 p.m., Andrei Sekretenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70671/
> -----------------------------------------------------------
>
> (Updated June 7, 2019, 4:21 p.m.)
>
>
> Review request for mesos, Benno Evers, Benjamin Mahler, and Joseph Wu.
>
>
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch introduces a class with mock methods for subscribing to the
> events of master's V1 streaming API and setting expectations for them.
>
>
> Diffs
> -----
>
> src/Makefile.am 93b2606841745c74dc33713e6b1b2fd62a1c7e74
> src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
> src/tests/master/mock_master_api_subscriber.hpp PRE-CREATION
> src/tests/master/mock_master_api_subscriber.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/70671/diff/6/
>
>
> Testing
> -------
>
> `./bin/mesos-tests.sh --verbose --gtest_filter="*UpdateFramework*" --gtest_break_on_failure --gtest_repeat=1000` with new tests from https://reviews.apache.org/r/70534/
>
> `./bin/mesos-tests.sh --verbose --gtest_filter="*MasterAPITest*" --gtest_break_on_failure --gtest_repeat=1000` with refactored tests from https://reviews.apache.org/r/70756/
>
>
> Thanks,
>
> Andrei Sekretenko
>
>
Re: Review Request 70671: Added a class for setting expectations on
master V1 API events in tests.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70671/
-----------------------------------------------------------
(Updated June 7, 2019, 4:21 p.m.)
Review request for mesos, Benno Evers, Benjamin Mahler, and Joseph Wu.
Changes
-------
Removed workaround for libprocess deadlock (which was fixed in MESOS-9808).
Bugs: MESOS-7258
https://issues.apache.org/jira/browse/MESOS-7258
Repository: mesos
Description
-------
This patch introduces a class with mock methods for subscribing to the
events of master's V1 streaming API and setting expectations for them.
Diffs (updated)
-----
src/Makefile.am 93b2606841745c74dc33713e6b1b2fd62a1c7e74
src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
src/tests/master/mock_master_api_subscriber.hpp PRE-CREATION
src/tests/master/mock_master_api_subscriber.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/70671/diff/6/
Changes: https://reviews.apache.org/r/70671/diff/5-6/
Testing
-------
`./bin/mesos-tests.sh --verbose --gtest_filter="*UpdateFramework*" --gtest_break_on_failure --gtest_repeat=1000` with new tests from https://reviews.apache.org/r/70534/
`./bin/mesos-tests.sh --verbose --gtest_filter="*MasterAPITest*" --gtest_break_on_failure --gtest_repeat=1000` with refactored tests from https://reviews.apache.org/r/70756/
Thanks,
Andrei Sekretenko
Re: Review Request 70671: Added a class for setting expectations on
master V1 API events in tests.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70671/
-----------------------------------------------------------
(Updated June 3, 2019, 8:28 p.m.)
Review request for mesos, Benno Evers, Benjamin Mahler, and Joseph Wu.
Bugs: MESOS-7258
https://issues.apache.org/jira/browse/MESOS-7258
Repository: mesos
Description
-------
This patch introduces a class with mock methods for subscribing to the
events of master's V1 streaming API and setting expectations for them.
Diffs
-----
src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f
src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
src/tests/master/mock_master_api_subscriber.hpp PRE-CREATION
src/tests/master/mock_master_api_subscriber.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/70671/diff/5/
Testing (updated)
-------
`./bin/mesos-tests.sh --verbose --gtest_filter="*UpdateFramework*" --gtest_break_on_failure --gtest_repeat=1000` with new tests from https://reviews.apache.org/r/70534/
`./bin/mesos-tests.sh --verbose --gtest_filter="*MasterAPITest*" --gtest_break_on_failure --gtest_repeat=1000` with refactored tests from https://reviews.apache.org/r/70756/
Thanks,
Andrei Sekretenko
Re: Review Request 70671: Added a class for setting expectations on
master V1 API events in tests.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70671/
-----------------------------------------------------------
(Updated June 3, 2019, 8:22 p.m.)
Review request for mesos, Benno Evers, Benjamin Mahler, and Joseph Wu.
Bugs: MESOS-7258
https://issues.apache.org/jira/browse/MESOS-7258
Repository: mesos
Description
-------
This patch introduces a class with mock methods for subscribing to the
events of master's V1 streaming API and setting expectations for them.
Diffs
-----
src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f
src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
src/tests/master/mock_master_api_subscriber.hpp PRE-CREATION
src/tests/master/mock_master_api_subscriber.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/70671/diff/5/
Testing (updated)
-------
./bin/mesos-tests.sh --verbose --gtest_filter="*UpdateFramework*" --gtest_break_on_failure --gtest_repeat=1000 with new tests from https://reviews.apache.org/r/70534/
./bin/mesos-tests.sh --verbose --gtest_filter="*MasterAPITest*" --gtest_break_on_failure --gtest_repeat=1000 with refactored tests from https://reviews.apache.org/r/70756/
Thanks,
Andrei Sekretenko