----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15540/#review29345 -----------------------------------------------------------
src/master/http.cpp <https://reviews.apache.org/r/15540/#comment56529> Maybe instead of saying 'running and completed tasks' we just say 'tasks'? Likewise, s/all active// since this is for completed frameworks too right? src/master/http.cpp <https://reviews.apache.org/r/15540/#comment56530> Again, I think we should just say 'tasks', and if we get specific say 'Lists known tasks.'. src/master/http.cpp <https://reviews.apache.org/r/15540/#comment56531> Please use markdown here (i.e., '>' not <pre>). See TOGGLE_HELP in libprocess/src/process.cpp for an example. src/master/http.cpp <https://reviews.apache.org/r/15540/#comment56532> Please put '{' on newline. src/master/http.cpp <https://reviews.apache.org/r/15540/#comment56533> Please put '{' on newline! ;) src/master/http.cpp <https://reviews.apache.org/r/15540/#comment56534> It's fine to be pedantic, but it should always be the case that the statuses are in order and the tail of the list should be the latest timestamp and the head of the list should be the earliest timestamp so we might as well simplify this (and even put a CHECK in place if you'd like to confirm). src/master/http.cpp <https://reviews.apache.org/r/15540/#comment56535> { src/master/http.cpp <https://reviews.apache.org/r/15540/#comment56537> This can just be: return lhs->statuses(0) < rhs->statuses(0); src/master/http.cpp <https://reviews.apache.org/r/15540/#comment56536> { src/master/http.cpp <https://reviews.apache.org/r/15540/#comment56538> Result<int> result = numify<int>(request.query.get("limit")); // Do you want to return an error if they gave you bad input? At the very least maybe a TODO? size_t limit = result.isSome() ? result.get() : defaultTasksLimit; src/master/http.cpp <https://reviews.apache.org/r/15540/#comment56539> You can cleanup this like the suggestion above too (can even reuse the 'result' variable). src/master/http.cpp <https://reviews.apache.org/r/15540/#comment56540> s/orderString/order/ src/master/master.hpp <https://reviews.apache.org/r/15540/#comment56541> This should be in src/master/constants.hpp and be in all caps. Also, no need to say 'default' since that's what most of our constants are (see examples in constants.hpp). - Benjamin Hindman On Nov. 18, 2013, 7:26 p.m., Niklas Nielsen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15540/ > ----------------------------------------------------------- > > (Updated Nov. 18, 2013, 7:26 p.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, and Ross Allen. > > > Repository: mesos-git > > > Description > ------- > > This patch lists all running and recent completed tasks, sorted by date > and starting at offset and limited by limit parameter. > > > Diffs > ----- > > src/master/http.cpp deb5c97 > src/master/master.hpp e377af8 > src/master/master.cpp abab6ce > > Diff: https://reviews.apache.org/r/15540/diff/ > > > Testing > ------- > > make check and functional testing > > > Thanks, > > Niklas Nielsen > >
