-----------------------------------------------------------
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
> 
>

Reply via email to