----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69064/#review210989 -----------------------------------------------------------
I've given a shot at making the `MasterLoadTest.SimultaneousBatchedRequests` deterministic. My goal is to get the master backlogged on purpose, because when the master's backlog looks like: ``` 1) HTTP GET /state 2) HTTP GET /state 3) HTTP GET /state 4) HTTP GET /state 5) HTTP GET /state 6) <Other event> ``` The master will be forced to process the 5 HTTP GETs in a row, which leads to 5 dispatches to the `ObjectApprover::create()`, which are chained to `deferBatchedRequest`. The `ObjectApprover::create()` runs on the Authorizer actor. We can tweak the Authorizer to wait for X number of `getObjectApprover` calls before it returns (all at once). If we do this correctly, the master's backlog should then look like: ``` 1) <Event blocking the master from the test body> 2) deferBatchedRequest(/state) 3) deferBatchedRequest(/state) 4) deferBatchedRequest(/state) 5) deferBatchedRequest(/state) 6) deferBatchedRequest(/state) ``` At which point, the test can unblock the (1) event and let the master process each batched request. The call to `processRequestsBatch` would be scheduled after all the `deferBatchedRequest`s, so there should be 4 cache hits. You can use the metric added in the next review to verify the cache hits. I'll post the major additions/changes below, roughly in-line below: src/tests/master_load_tests.cpp Lines 73-101 (patched) <https://reviews.apache.org/r/69064/#comment295820> In terms of helpers, I added a replacement Authorizer: ``` // This authorizer is like the `MockAuthorizer`, which returns an // `AcceptingObjectApprover` for every authorization request, i.e. // everybody is authorized to view everything by default. // // The difference is that this authorizer will not satisfy any futures // from `getObjectApprover` until it is told to, presumably from the test body. // The class is basically a giant gate for certain requests. class BlockingAuthorizerProcess : public process::Process<BlockingAuthorizerProcess> { public: BlockingAuthorizerProcess() : ProcessBase(process::ID::generate("blocking-authorizer")), blocked(true) {} // NOTE: Should be unused in this test. Future<bool> authorized(const authorization::Request& request) { return true; } Future<Owned<ObjectApprover>> getObjectApprover( const Option<authorization::Subject>& subject, const authorization::Action& action) { if (blocked) { promises.emplace(); return promises.back().future(); } return Owned<ObjectApprover>(new AcceptingObjectApprover()); } // Returns the number of pending calls to `getObjectApprover`. Future<size_t> pending() { return promises.size(); } // Satisfies all future and prior calls made to `getObjectApprover`. Future<Nothing> unleash() { while (!promises.empty()) { promises.front().set( Owned<ObjectApprover>(new AcceptingObjectApprover())); promises.pop(); } blocked = false; return Nothing(); } private: std::queue<Promise<Owned<ObjectApprover>>> promises; bool blocked; }; class BlockingAuthorizer : public Authorizer { public: BlockingAuthorizer() : process(new BlockingAuthorizerProcess()) { process::spawn(process.get()); } ~BlockingAuthorizer() { process::terminate(process.get()); process::wait(process.get()); } Future<bool> authorized(const authorization::Request& request) override { return process::dispatch( process.get(), &BlockingAuthorizerProcess::authorized, request); } Future<Owned<ObjectApprover>> getObjectApprover( const Option<authorization::Subject>& subject, const authorization::Action& action) override { return process::dispatch( process.get(), &BlockingAuthorizerProcess::getObjectApprover, subject, action); } Future<size_t> pending() { return process::dispatch( process.get(), &BlockingAuthorizerProcess::pending); } Future<Nothing> unleash() { return process::dispatch( process.get(), &BlockingAuthorizerProcess::unleash); } private: Owned<BlockingAuthorizerProcess> process; }; ``` src/tests/master_load_tests.cpp Lines 152-155 (patched) <https://reviews.apache.org/r/69064/#comment295821> I replaced this with a `BlockingAuthorizer authorizer;` src/tests/master_load_tests.cpp Lines 186-198 (patched) <https://reviews.apache.org/r/69064/#comment295824> In my attempt, I did not try to launch multiple requests on multiple threads. Instead, I (messily/hackily/lazily) just sent all the requests in the test body. ``` // Send all the HTTP requests. std::vector<const char*> endpoints { "/state", "/state-summary", "/frameworks", "/slaves", "/roles", "/state", "/state-summary", "/frameworks", "/slaves", "/roles", "/state", "/state-summary", "/frameworks", "/slaves", "/roles", "/state", "/state-summary", "/frameworks", "/slaves", "/roles", "/state", "/state-summary", "/frameworks", "/slaves", "/roles", "/state", "/state-summary", "/frameworks", "/slaves", "/roles", "/state", "/state-summary", "/frameworks", "/slaves", "/roles", "/state", "/state-summary", "/frameworks", "/slaves", "/roles", "/state", "/state-summary", "/frameworks", "/slaves", "/roles", "/state", "/state-summary", "/frameworks", "/slaves", "/roles", }; process::http::Headers headers = createBasicAuthHeaders(DEFAULT_CREDENTIAL); std::vector<Future<Response>> responses; foreach (const char* endpoint, endpoints) { responses.push_back( process::http::get(master.get()->pid, endpoint, "", headers)); } ``` If we write the other parts of the test correctly, it shouldn't matter how the requests are made. src/tests/master_load_tests.cpp Lines 200-203 (patched) <https://reviews.apache.org/r/69064/#comment295825> This is how I ensure there will be some cache hits: ``` // Wait until all the HTTP events have reached the master and are now // awaiting authorization. There might be some other requests that get // mixed into the authorizer, so we must have ample requests in the // test body to ensure cache hits. Future<size_t> pendingHttpCalls; do { pendingHttpCalls = authorizer.pending(); AWAIT_READY(pendingHttpCalls); } while (pendingHttpCalls.get() < 50u); ``` Note: This method of blocking the master means you cannot pause the clock, as that may deadlock the master process when the `await()` spawns a latch and then `ProcessManager::settle()`s. The `settle()` will never complete since the master is "running". Using a non-libprocess way of blocking would not deadlock. ``` // Now block the master actor, since we don't want the master to start // batching until it is queued up with all the HTTP requests again. Promise<Nothing> masterBlocker; process::dispatch(master.get()->pid, [&masterBlocker]() { masterBlocker.future().await(); }); ``` This part is still a little racy, in terms of the exact number of requests that make it onto the master's queue before we un-gate the Authorizer. ``` // Unblock the BlockingAuthorizer. // This should trigger all the deferals onto the master from the Authorizer's // thread. When this future completes, the master's queue should be full // of batched requests. AWAIT_READY(authorizer.unleash()); // Unblock the master now, so it can perform the batching. masterBlocker.set(Nothing()); // Unblock the master and let the batched HTTP handler do its magic. AWAIT_READY(process::collect(responses)); ``` After this, you can check the response as you've done. Plus check cache hits. - Joseph Wu On Nov. 21, 2018, 5:22 p.m., Benno Evers wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69064/ > ----------------------------------------------------------- > > (Updated Nov. 21, 2018, 5:22 p.m.) > > > Review request for mesos, Alexander Rukletsov and Joseph Wu. > > > Repository: mesos > > > Description > ------- > > This commit adds a set of unit test to verify that > basic Master HTTP endpoints still work correctly > under the presence of request caching. > > > Diffs > ----- > > src/Makefile.am 8da1a05b618f17542fec9b5057484a9bae57658a > src/tests/master_load_tests.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/69064/diff/3/ > > > Testing > ------- > > > Thanks, > > Benno Evers > >
