----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27113/#review80540 -----------------------------------------------------------
Ship it! Wow, nice work, this is a lot cleaner! I left some comments but since they are pretty minor, I've gone ahead and taken care of them to get this committed for you. For the dependent change of disabling local messages, I've left it as a TODO here so we can get these great cleanups in. Let's follow up with your dependent patch to add the ability for the tests to run both the local and remote code paths, sound good? :) 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment130567> Looks like we're missing some includes here? 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment130574> Weird wrapping here? You could make this comment more succinct here if you omit the request parameters. We probably don't need a /help for now. 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment130572> Maybe a newline in between these? 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment130563> It seems a bit strange to have a `running` that isn't set to false? 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment130570> Since we're capturing the duration for the run, how about just calling it 'duration'? Finished makes it clear that it's a signal for finishing but that seems to express "how" it's used as opposed to "what" it is. 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment130575> It's probably time we give this a more meaningful name than "Test". :) 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment130561> No need for `&` here. 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment130568> How about: ``` // Start the ping / pongs! const string query = strings::join( "&", "server=" + stringify(serverPid), "requests=" + stringify(numRequests), "concurrency=" + stringify(concurrency), "messageSize=" + stringify(messageSize)); ``` 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment130559> Can we use std::array in the future? 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment130543> Hm.. we call this 'requests' but we store 'Responses'? 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment130548> It seems like the "iterate and wait ..." aspect of this comment re-iterates what the code says? 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment130556> A 'request' is actually a 'Response'? 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment130553> Printing the total rpc throughput like this seems prone to error. For example, if each client ran __serially__ at throughput X, we would print X*N without realizing that each client's throughput was not overlapping. Could we just print the throughput for each individual client for now? For server throughput, we can track that more precisely in the server itself, yes? For now, I'd propose just tracking the total time in the test and estimating total throughput based on that. Also, why not use cout like we did elsewhere? - Ben Mahler On April 17, 2015, 5:28 p.m., Joris Van Remoortere wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27113/ > ----------------------------------------------------------- > > (Updated April 17, 2015, 5:28 p.m.) > > > Review request for mesos, Ben Mahler, Cody Maloney, Joerg Schad, and Michael > Park. > > > Bugs: MESOS-1980 > https://issues.apache.org/jira/browse/MESOS-1980 > > > Repository: mesos > > > Description > ------- > > Clean up Libprocess for readability / extensibility. > > > Diffs > ----- > > 3rdparty/libprocess/src/tests/benchmarks.cpp > a927e4ecd8c8955cd9f85e716173a73a9a21c6cd > > Diff: https://reviews.apache.org/r/27113/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Joris Van Remoortere > >
