mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anand Mazumdar <mazumdar.an...@gmail.com>
Subject Re: Review Request 48049: Implemented GET_LEADING_MASTER Call in v1 master API.
Date Mon, 30 May 2016 16:59:44 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48049/#review135525
-----------------------------------------------------------



LGTM, just one minor issue where we could benefit from implementing the functionality in `evolve`.


src/master/http.cpp (line 1192)
<https://reviews.apache.org/r/48049/#comment200525>

    New line before. We use 2 new lines to separate elements outside classes.



src/master/http.cpp (line 1201)
<https://reviews.apache.org/r/48049/#comment200520>

    New line before



src/master/http.cpp (lines 1204 - 1214)
<https://reviews.apache.org/r/48049/#comment200523>

    hmm.. Can't you just implement a evolve function that converts from `MasterInfo` to `v1::MasterInfo`
and then just use it here?



src/master/http.cpp (line 1210)
<https://reviews.apache.org/r/48049/#comment200521>

    New line before



src/master/http.cpp (line 1220)
<https://reviews.apache.org/r/48049/#comment200526>

    Delete a extra new line here



src/tests/api_tests.cpp (line 179)
<https://reviews.apache.org/r/48049/#comment200527>

    This was already fixed by bbannier in `cb1fbdaa4`. Can you rebase?



src/tests/api_tests.cpp (line 181)
<https://reviews.apache.org/r/48049/#comment200529>

    New line here



src/tests/api_tests.cpp (line 183)
<https://reviews.apache.org/r/48049/#comment200530>

    Why the extra new line here?



src/tests/api_tests.cpp (line 197)
<https://reviews.apache.org/r/48049/#comment200533>

    The `get()` here is redundant. 
    
    Can you just do:
    
    `v1Response->IsInitialized()` and in the other two places in this test.
    
    I know the other tests already use it in this file but it would be good to not introduce
new ones.  I would clean up the rest in another review.



src/tests/api_tests.cpp (line 199)
<https://reviews.apache.org/r/48049/#comment200532>

    Can you swap the order of arguments i.e.
    
    `master.get()->getMasterInfo().ip())` is the the expected argument.



src/tests/api_tests.cpp (line 345)
<https://reviews.apache.org/r/48049/#comment200528>

    Ditto, rebase


- Anand Mazumdar


On May 30, 2016, 4:38 p.m., Abhishek Dasgupta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48049/
> -----------------------------------------------------------
> 
> (Updated May 30, 2016, 4:38 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5497
>     https://issues.apache.org/jira/browse/MESOS-5497
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented GET_LEADING_MASTER Call in v1 master API and
> made a minor change in the test case for GET_LOGGING_LEVEL.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp c8d2f46d9e0ad8a99a6ebffc6a3d5d852cee0616 
>   src/master/master.hpp eeeccdfdfd296c2a484764e887564f2e065cfd14 
>   src/tests/api_tests.cpp 1d7a3a9df395805c250148bf6232b00d712c7a05 
> 
> Diff: https://reviews.apache.org/r/48049/diff/
> 
> 
> Testing
> -------
> 
> On Ubuntu 16.04:
> 
> sudo GTEST_FILTER="*MasterAPITest*.*" make -j2 check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message