----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41188/#review110387 -----------------------------------------------------------
LGTM, Just some minor style nits and suggestions. src/tests/slave_tests.cpp (line 2147) <https://reviews.apache.org/r/41188/#comment170240> I am assuming that we are testing that port labels are populated correctly in the `DiscoveryInfo` object. How about: ``` // This test verifies that port labels are exposed over the slave state endpoint. ``` src/tests/slave_tests.cpp (line 2149) <https://reviews.apache.org/r/41188/#comment170243> s/DiscoveryInfoAndPorts/PortLabels src/tests/slave_tests.cpp (line 2178) <https://reviews.apache.org/r/41188/#comment170232> ``` labels->add_labels()->CopyFrom(createLabel("vip1", "192.168.1.1:80")); ``` Ditto for the other occurence. src/tests/slave_tests.cpp (line 2188) <https://reviews.apache.org/r/41188/#comment170203> Kill this new line. We generally use `one-line` newlines to separate code-blocks. src/tests/slave_tests.cpp (line 2190) <https://reviews.apache.org/r/41188/#comment170244> How about: ``` Port *port1 = ports.add_ports(); port1->set_number(80); port1->mutable_labels()->CopyFrom(labels1); Port *port2 = ports.add_ports(); port2->set_number(8081); port2->mutable_labels()->CopyFrom(labels2); ``` This makes it a bit cleaner. src/tests/slave_tests.cpp (line 2197) <https://reviews.apache.org/r/41188/#comment170234> Delete this comment. Also PS, we start comments with one space and the first character should be `capital` ``` // Set DiscoveryInfo in the TaskInfo ``` src/tests/slave_tests.cpp (line 2202) <https://reviews.apache.org/r/41188/#comment170245> New line before. src/tests/slave_tests.cpp (line 2204) <https://reviews.apache.org/r/41188/#comment170204> Kill this newline. src/tests/slave_tests.cpp (line 2205) <https://reviews.apache.org/r/41188/#comment170215> Why not just do this: ``` EXPECT_CALL(exec, registered(_, _, _, _, _)); ``` AFAICT, you don't seem to be using any of the arguments from the `registered` callback. All you are interested in is that the expectation is fullfilled exactly once. src/tests/slave_tests.cpp (line 2209) <https://reviews.apache.org/r/41188/#comment170208> You don't seem to be using `execTask` i.e. `TaskInfo` attributed. Why not just do: ``` Future<Nothing> launchTask; EXPECT_CALL(exec, launchTask(_, _)) .WillOnce(FutureSatisfy(&launchTask)); ``` and then later do ```AWAIT_READY(launchTask);``` src/tests/slave_tests.cpp (line 2220) <https://reviews.apache.org/r/41188/#comment170216> New line here. We generally insert a newline after a statement spanning multiple lines. src/tests/slave_tests.cpp (line 2222) <https://reviews.apache.org/r/41188/#comment170224> We generally avoid magic string constants. Can you just do ``` ASSERT_SOME_EQ( APPLICATION_JSON, response.get().headers.get("Content-Type")); ``` Can we also check if the response code is OK() ? src/tests/slave_tests.cpp (line 2228) <https://reviews.apache.org/r/41188/#comment170246> Kill this comment. src/tests/slave_tests.cpp (line 2234) <https://reviews.apache.org/r/41188/#comment170247> Kill this comment. `EXPECT_EQ` is self explanatory. src/tests/slave_tests.cpp (line 2238) <https://reviews.apache.org/r/41188/#comment170253> How about: ``` // Check that ports are set in the `DiscoveryInfo` object. ``` src/tests/slave_tests.cpp (line 2243) <https://reviews.apache.org/r/41188/#comment170252> Newline before. src/tests/slave_tests.cpp (line 2245) <https://reviews.apache.org/r/41188/#comment170251> New line before this line. Also, `// Verify that the ports match.` src/tests/slave_tests.cpp (line 2250) <https://reviews.apache.org/r/41188/#comment170218> Kill all the newlines. Just have one newline - Anand Mazumdar On Dec. 14, 2015, 11:34 p.m., Avinash sridharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41188/ > ----------------------------------------------------------- > > (Updated Dec. 14, 2015, 11:34 p.m.) > > > Review request for mesos, Adam B and Anand Mazumdar. > > > Bugs: MESOS-3962 > https://issues.apache.org/jira/browse/MESOS-3962 > > > Repository: mesos > > > Description > ------- > > Providing JSON bindings to so that mesos modules can expose DiscoveryInfo > protobuf messages to HTTP endpoints > > > Diffs > ----- > > src/slave/http.cpp d1b1158c0a80d72c32c9b28977043b6be2295239 > src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 > src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 > > Diff: https://reviews.apache.org/r/41188/diff/ > > > Testing > ------- > > make check. > Added Unit tests to verify setting of DiscoveryInfo in state.json in slave. > Also added Unit test to test that DiscoveryInfo gets exposed in master when > TaskInfo protobuf is converted to JSON objects. > > > Thanks, > > Avinash sridharan > >
