----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27113/#review80101 -----------------------------------------------------------
Wow, the client and server parts of this are looking a lot clearer, thanks Joris! Let's focus on the test body next, it was difficult for me to grasp it fully, I left a comment below :) 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment129861> How about 'concurrency' or 'concurrentRequests' instead of 'queueDepth'? Which "queue" is this variable referring to? 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment129863> This seems to suggest support for multiple "runs" of the client. However, if a /run request arrives while the previous run is in progress, it looks like this will behave strangely. Did you want to deny requests while a run is in progress, or chain the runs after one another? 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment129864> Why do totalRequests and the concurrency factor need to be reset here? Looks like these are required. 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment129871> How about: ``` hashmap<string, Option<string>> parameters { {"server", request.query.get("server")}, {"messageSize", request.query.get("messageSize")}, {"totalRequests", request.query.get("totalRequests")}, {"concurrentRequests", request.query.get("concurrentRequests")}, }; // Ensure all parameters were provided. foreachpair (const string& parameter, const Option<string>& value, parameters) { if (value.isNone()) { return http::BadRequest("Missing '" + parameter + "' parameter"); } } server = UPID(parameters["server"].get()); Try<Bytes> bytes = Bytes::parse(parameters["messageSize"].get()); if (bytes.isError()) { return http::BadRequest("Invalid 'messageSize': " + bytes.error()); } messageSize = bytes.get(); Try<size_t> numify = numify<size_t>(parameters["totalRequests"].get()); if (numify.isError()) { return http::BadRequest("Invalid 'totalRequests': " + numify.error()); } totalRequests = numify.get(); numify = numify<size_t>(parameters["concurrentRequests"].get()); if (numify.isError()) { return http::BadRequest("Invalid 'concurrentRequests': " + numify.error()); } concurrentRequests = numify.get(); ``` 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment129865> How about using a `Bytes` for this? ``` Try<Bytes> messageSize = Bytes::parse(value.get()); if (messageSize.isError()) { } message = string(messageSize.get().bytes(), '1'); 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment129866> Can you include the error message from the Try here and elsewhere? 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment129873> Why do you need to reject this? If the number of concurrent requests is larger than the total number of requests, that seems well defined (concurrentRequests == totalRequests). Any reason not to just do the right thing for what was requested? 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment129875> 'concurrency' or 'concurrentRequests' might make this clearer 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment129877> Comment seems unnecessary? 3rdparty/libprocess/src/tests/benchmarks.cpp <https://reviews.apache.org/r/27113/#comment129881> Could you do a self review of this test body, from the perspective of someone approaching this code for the first time? I had a hard time understanding this, at a basic level what is the overall structure of the forking here? It's also a bit tricky to figure out the lower level details, like what the `requestPipes` and `resultPipes` are for, and why they're needed. What the vectors of pipes are used for, why they're needed. Etc. In the process of being able to articulate what this does, we might figure out how we can make it more easily understandable :) - Ben Mahler On April 14, 2015, 7:46 p.m., Joris Van Remoortere wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27113/ > ----------------------------------------------------------- > > (Updated April 14, 2015, 7:46 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 > >
