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