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 Wed, 29 May 2019 18:02:53 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70671/#review215579
-----------------------------------------------------------



Overall this is looking pretty good, thanks Andrei!


src/Makefile.am
Lines 2615-2616 (patched)
<https://reviews.apache.org/r/70671/#comment302303>

    alphabetical?
    
    Can you line up the backslashes with the other lines?



src/tests/CMakeLists.txt
Lines 55 (patched)
<https://reviews.apache.org/r/70671/#comment302304>

    alphabetical?



src/tests/master/mock_master_api_subscriber.hpp
Lines 72-73 (patched)
<https://reviews.apache.org/r/70671/#comment302315>

    let's not mention the implementation detail of the libprocess loop here



src/tests/master/mock_master_api_subscriber.hpp
Lines 78-80 (patched)
<https://reviews.apache.org/r/70671/#comment302316>

    Can we introduce a little bit of state to prevent this more than 1 call to subscribe?



src/tests/master/mock_master_api_subscriber.cpp
Lines 51-57 (patched)
<https://reviews.apache.org/r/70671/#comment302313>

    if you use .then you can return directly and won't need the CHECK(!failed):
    
    ```
    return process::http::streaming::post(
        pid,
        "api/v1",
        headers,
        serialize(contentType, call),
        stringify(contentType))
      .then(defer(self(), &Self::_subscribe, lambda::_1));
    ```
    
    See comment below, this lets us then surface errors to the test and the test can wait
and ASSERT for a correct connection rather than having the implicit CHECKs crash the whole
test binary.



src/tests/master/mock_master_api_subscriber.cpp
Lines 65-68 (patched)
<https://reviews.apache.org/r/70671/#comment302308>

    Rather than crashing the program here, let's return a failure to the caller of subscribe
if there's an error. And the test will ASSERT that the future coming out of subscribe succeeded.



src/tests/master/mock_master_api_subscriber.cpp
Lines 82 (patched)
<https://reviews.apache.org/r/70671/#comment302314>

    This seems a bit hard for the reader, consider:
    
    ```
        Owned<Reader<Event>> reader = ...;
    
        process::loop(
            self(),
            [=]() {
              return reader->read();
            },
    ```



src/tests/master/mock_master_api_subscriber.cpp
Lines 85 (patched)
<https://reviews.apache.org/r/70671/#comment302311>

    "Received EOF from master streaming API"
    
    no periods in log messages



src/tests/master/mock_master_api_subscriber.cpp
Lines 89-92 (patched)
<https://reviews.apache.org/r/70671/#comment302310>

    Can you put this first before EOF? That would be more consistent with how we tend to handle
errors first



src/tests/master/mock_master_api_subscriber.cpp
Lines 90 (patched)
<https://reviews.apache.org/r/70671/#comment302312>

    Failed to read master streaming API event: ...



src/tests/master/mock_master_api_subscriber.cpp
Lines 94 (patched)
<https://reviews.apache.org/r/70671/#comment302309>

    Isn't this going to print the number rather than the string? (i.e. 1 instead of ENUM_NAME)
    
    Or do we have a << overload?
    
    "Received SUBSCRIBE from master streaming API" (i.e. a bit more consistent with the EOF
logging, see suggestion above)



src/tests/master/mock_master_api_subscriber.cpp
Lines 131-134 (patched)
<https://reviews.apache.org/r/70671/#comment302306>

    If possible, we prefer to use the "managed=true" approach to avoid self-wait deadlocks:
    
    ```
    PID<MockMasterAPISubscriberProcess> process =
      spawn(new MockMasterAPISubscriberProcess(this), true);
      
    ...
    
    terminate(process);
    // no need to wait
    ```
    
    However, it looks like that's not possible here since the process holds a back pointer
to `this`? Perhaps it's worth noting that:
    
    ```
      // Note that we must wait for the process to terminate before we leave
      // the destructor (and therefore cannot spawn the process with `managed=true`)
      // since the process may invoke the mock methods on this class.
      terminate(process.get());
      wait(process.get());
    ```



src/tests/master/mock_master_api_subscriber.cpp
Lines 141-143 (patched)
<https://reviews.apache.org/r/70671/#comment302305>

    Can you remove the logging here?



src/tests/master/mock_master_api_subscriber.cpp
Lines 162-165 (patched)
<https://reviews.apache.org/r/70671/#comment302307>

    Can you put this case at the bottom and remove the UNREACHABLE()? (We usually treat LOG(FATAL)
as terminating without adding UNREACHABLE to guard against it somehow not terminating).


- Benjamin Mahler


On May 28, 2019, 5:35 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70671/
> -----------------------------------------------------------
> 
> (Updated May 28, 2019, 5:35 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> 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/3/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>


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