mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vinod Kone <vinodk...@gmail.com>
Subject Re: Review Request 48751: Implement GetState response for master API.
Date Tue, 21 Jun 2016 01:29:32 GMT

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




include/mesos/v1/master.proto (lines 181 - 193)
<https://reviews.apache.org/r/48751/#comment203931>

    How about defining *top level* `Framework` and maybe `Executor` message? I think it could
be reused by GET_FRAMEWORKS and GET_AGENTS calls?
    
    Infact, if Framework didn't contain `offers` we can move `Framework` and `Executor` to
mesos.proto right next to `Task`.



include/mesos/v1/mesos.proto (line 1303)
<https://reviews.apache.org/r/48751/#comment203910>

    can you rebase? v1::Task is now committed.



src/internal/evolve.hpp (line 59)
<https://reviews.apache.org/r/48751/#comment203911>

    this should already be there.



src/master/http.cpp (line 1386)
<https://reviews.apache.org/r/48751/#comment203914>

    this should take unversioned call now.



src/master/http.cpp (lines 1392 - 1393)
<https://reviews.apache.org/r/48751/#comment203917>

    no need for this. kill it.



src/master/http.cpp (lines 1397 - 1401)
<https://reviews.apache.org/r/48751/#comment203913>

    Use unversioned master::Response and do evolve just before `return`ing. This is the pattern
used by other handlers now.
    
    ```
    return OK(serialize(contentType, evolve(response),
              stringify(contentType);
    ```



src/master/http.cpp (line 1410)
<https://reviews.apache.org/r/48751/#comment203916>

    indent.



src/master/http.cpp (lines 1410 - 1411)
<https://reviews.apache.org/r/48751/#comment203918>

    kill it.



src/master/http.cpp (line 1522)
<https://reviews.apache.org/r/48751/#comment203924>

    put this on the line above
    
    ```
    foreachpair(arg1,
                arg2,
                arg3)
    ```



src/master/http.cpp (line 1547)
<https://reviews.apache.org/r/48751/#comment203925>

    indentation.



src/tests/api_tests.cpp (line 244)
<https://reviews.apache.org/r/48751/#comment203927>

    It's not clear what the type of `framework` is, so I would recommend calling out the type
explicitly instead of using `auto`.
    
    also add a new line before this.



src/tests/api_tests.cpp (line 292)
<https://reviews.apache.org/r/48751/#comment203929>

    see above.



src/tests/api_tests.cpp (line 328)
<https://reviews.apache.org/r/48751/#comment203928>

    see abovwe.


- Vinod Kone


On June 15, 2016, 8:52 p.m., Zhitao Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48751/
> -----------------------------------------------------------
> 
> (Updated June 15, 2016, 8:52 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5489
>     https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implement GetState response for master API, which should be functional equivalent (but
not necessarily fully compatible on json with `/state` endpoint).
> 
> The response protobuf message and new function `_getState` will be reused for generating
snapshot for `Subscribe` call in next patch.
> 
> Also copied `Task` message to V1 API.
> 
> 
> Diffs
> -----
> 
>   include/mesos/v1/master.proto 7b07b90557e0202cabc8f6164582a058631ab0e8 
>   include/mesos/v1/mesos.proto 39967fa09d2774d564f3df28277edea8ebcfb50d 
>   src/internal/evolve.hpp 66a3deaa94939ad2233d944ba35ac7e5cbe682e7 
>   src/internal/evolve.cpp 7f16cbda7da6c838648cca909368973e7298730b 
>   src/master/http.cpp db625f0d656f207a89fcc14b18ae2fc31d30e673 
>   src/master/master.hpp a0944ddccd3a4b33458cd2489bb5fcdbbdc55720 
>   src/tests/api_tests.cpp 3a482ca2a640b3f3e3b08a80ac84068d7e9ff8b0 
> 
> Diff: https://reviews.apache.org/r/48751/diff/
> 
> 
> Testing
> -------
> 
> New test in MasterAPITest.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>


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