----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68131/#review210347 -----------------------------------------------------------
Fix it, then Ship it! src/tests/master_benchmarks.cpp Line 493 (original), 503 (patched) <https://reviews.apache.org/r/68131/#comment295016> Do we still need the `Multiclient` suffix if we're not going to commit the `Delay` version? src/tests/master_benchmarks.cpp Line 598 (original), 605 (patched) <https://reviews.apache.org/r/68131/#comment295007> As far as I can tell, we only use `ATOMIC_VAR_INIT` in libprocess, in Mesos the usual constructor is used. (see e.g. the atomic `offerCallbacks` in `hierarchical_allocator_tests.cpp`) Also, per our style guide `std::atomic_bool` is preferred over `std::atomic<bool>`. src/tests/master_benchmarks.cpp Line 600 (original), 607 (patched) <https://reviews.apache.org/r/68131/#comment295008> s/exist/exit/ src/tests/master_benchmarks.cpp Line 604 (original), 611 (patched) <https://reviews.apache.org/r/68131/#comment295010> %s/repeateRequests/repeatedRequests/ src/tests/master_benchmarks.cpp Lines 616 (patched) <https://reviews.apache.org/r/68131/#comment295013> Last time we tried running this benchmark, we discovered a dead-lock caused by the interaction between the use of libprocess worker threads in both our test code and in the mesos code. Ideally we wouldn't use libprocess at all here, but to avoid another big change late in the review cycle, adding a check like this should be sufficient: ``` if (numClients >= LIBPROCESS_NUM_WORKER_THREADS - 3) { cerr << "Not enough worker threads for the desired number of clients."; exit(1); } ``` src/tests/master_benchmarks.cpp Lines 663 (patched) <https://reviews.apache.org/r/68131/#comment295009> You should probably check that the future wasn't failed before calling `.get()`. src/tests/master_benchmarks.cpp Line 640 (original), 674-677 (patched) <https://reviews.apache.org/r/68131/#comment295014> I'm a bit confused by the intention behind the stop condition: My best guess is that you want to ensure that you have constant load during the whole duration of the benchmark. However, in that case it seems like the requests to `/state` should not be limited to a specific number but continue indefinitely, and `stop` should be set to true after the required number of requests to the indicator endpoint have happened. On the other hand, if the current implementation is as intended, I think the message should read `launching *up to* {numRequests} requests`, because there's no guarantee that the loop with requests for the indicator endpoint will be the one finishing first. Also, in this case I think the comments could be a bit more specific about the intention. src/tests/master_benchmarks.cpp Lines 693 (patched) <https://reviews.apache.org/r/68131/#comment295012> I would advise against using `auto` here, since figuring out the actual type is not trivial. (one technically has to go the declaration of `process::collect()` in a different file) src/tests/master_benchmarks.cpp Line 646 (original), 702 (patched) <https://reviews.apache.org/r/68131/#comment295015> Given that we're capturing and printing baseline statistics anyways, would it make sense to print the relative slowdown between baseline performance and performance under load here, to get a metric that is comparable across Mesos versions? - Benno Evers On Nov. 4, 2018, 4:31 a.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68131/ > ----------------------------------------------------------- > > (Updated Nov. 4, 2018, 4:31 a.m.) > > > Review request for mesos, Benno Evers and Benjamin Mahler. > > > Bugs: MESOS-8975 > https://issues.apache.org/jira/browse/MESOS-8975 > > > Repository: mesos > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/tests/master_benchmarks.cpp b6d6dc7c1752491e2da854018966374b624d6682 > > > Diff: https://reviews.apache.org/r/68131/diff/4/ > > > Testing > ------- > > See https://reviews.apache.org/r/68132/ > > > Thanks, > > Alexander Rukletsov > >
