mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 70671: Added a class for setting expectations on master V1 API events in tests.
Date Mon, 10 Jun 2019 03:08:04 GMT

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


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