mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrei Sekretenko <asekrete...@mesosphere.io>
Subject Re: Review Request 70671: Added a class for setting expectations on master V1 API events in tests.
Date Wed, 12 Jun 2019 14:01:37 GMT


> 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
> 
>


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