----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29883/#review70034 -----------------------------------------------------------
A high level comment: we should probably split this patch into one for the slaves endpoint and one for the framework one :) src/master/http.cpp <https://reviews.apache.org/r/29883/#comment114978> Why static const and not just const? Also, why the capitilization? src/master/http.cpp <https://reviews.apache.org/r/29883/#comment114979> Would you mind throwing in a comment on how the parser will fill this out? I had to reread the code below a couple of times to make sense of it, which was maybe a hint that there should be some supporting documentation :) src/master/http.cpp <https://reviews.apache.org/r/29883/#comment114988> One thought: we already have helpers to let us model JSON from various Mesos structs and protobuf types. I wonder if, for consistency, we could do the same thing here. This would mean we could use something like: return OK(model(master->frameworks.registered), ...); instead. src/master/http.cpp <https://reviews.apache.org/r/29883/#comment114989> Same thing for tasks: we could have a model overload which took a vector of tasks and modeled them. That way, we could get rid of the tasks() overloads too. Thoughts? src/master/http.cpp <https://reviews.apache.org/r/29883/#comment114981> How about using the model() for task? https://github.com/apache/mesos/blob/master/src/common/http.hpp#L43 src/master/http.cpp <https://reviews.apache.org/r/29883/#comment114968> tasks() has a bit of an awkward overload (taken the existing endpoint). Let's think about if we can seperate the two somehow. src/master/http.cpp <https://reviews.apache.org/r/29883/#comment114980> Same thing here src/master/master.cpp <https://reviews.apache.org/r/29883/#comment114990> Let's see if we can get rid of the overloads instead :) src/master/master.cpp <https://reviews.apache.org/r/29883/#comment114991> Let's set the bar and add endpoint documentation up-front src/master/master.cpp <https://reviews.apache.org/r/29883/#comment114992> Same here. src/tests/master_tests.cpp <https://reviews.apache.org/r/29883/#comment114973> One more newline. src/tests/master_tests.cpp <https://reviews.apache.org/r/29883/#comment114974> Mind throwing in comments for the tests you are adding. They should look something like this: https://github.com/apache/mesos/blob/master/src/tests/master_tests.cpp#L2685 src/tests/master_tests.cpp <https://reviews.apache.org/r/29883/#comment114972> One more newline src/tests/master_tests.cpp <https://reviews.apache.org/r/29883/#comment114997> s/Some/Two/ src/tests/master_tests.cpp <https://reviews.apache.org/r/29883/#comment115001> If you are not using the vector to pass the messages around, we have traditionally numbered the variables: https://github.com/apache/mesos/blob/master/src/tests/master_tests.cpp#L1300 and https://github.com/apache/mesos/blob/master/src/tests/master_tests.cpp#L1327 src/tests/master_tests.cpp <https://reviews.apache.org/r/29883/#comment115000> Where do you use 'slaves'? src/tests/master_tests.cpp <https://reviews.apache.org/r/29883/#comment114971> One more newline src/tests/master_tests.cpp <https://reviews.apache.org/r/29883/#comment114970> One more newline src/tests/master_tests.cpp <https://reviews.apache.org/r/29883/#comment115002> See comment below. We could, for clarity, compare against a JSON string we put together and parse. src/tests/master_tests.cpp <https://reviews.apache.org/r/29883/#comment115005> Could we use join() instead? Here and below for clarity in what goes in the request path. src/tests/master_tests.cpp <https://reviews.apache.org/r/29883/#comment115006> Can we use createTasks() here? Take a look at https://github.com/apache/mesos/blob/master/src/tests/status_update_manager_tests.cpp#L141 src/tests/master_tests.cpp <https://reviews.apache.org/r/29883/#comment114998> s/ran/run/ src/tests/master_tests.cpp <https://reviews.apache.org/r/29883/#comment114996> How about writing the expected json string, parse it and compare with the received one? Here is a place where we are comparing with that approach: https://github.com/apache/mesos/blob/master/src/tests/master_tests.cpp#L2643 src/tests/master_tests.cpp <https://reviews.apache.org/r/29883/#comment115007> See comment above about join() src/tests/master_tests.cpp <https://reviews.apache.org/r/29883/#comment115004> Same thing here. src/tests/master_tests.cpp <https://reviews.apache.org/r/29883/#comment114969> One more newline - Niklas Nielsen On Jan. 16, 2015, 5:37 a.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29883/ > ----------------------------------------------------------- > > (Updated Jan. 16, 2015, 5:37 a.m.) > > > Review request for mesos, Ben Mahler, Niklas Nielsen, and Till Toenshoff. > > > Bugs: MESOS-2157 > https://issues.apache.org/jira/browse/MESOS-2157 > > > Repository: mesos-git > > > Description > ------- > > Adds endpoints for paths /master/slaves and > /master/frameworks/{framework}/tasks/{task} > > Builds on the abandoned patch 14286. > > Old description: this is WIP that bmahler wants to take on. > > > Diffs > ----- > > src/master/http.cpp 46890be > src/master/master.hpp 26116af > src/master/master.cpp 63ca19a > src/tests/master_tests.cpp 678d27f > > Diff: https://reviews.apache.org/r/29883/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Alexander Rojas > >
