You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Till Toenshoff <to...@me.com> on 2018/05/03 02:58:52 UTC
Re: Review Request 66229: Implemented UPDATE_FRAMEWORK call.

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



As discussed in our direct review, some more changes on the future-continuations are needed - left those out here.


src/master/validation.cpp
Line 526 (original), 526 (patched)
<https://reviews.apache.org/r/66229/#comment284093>

    Sorry I wasn't clear ... 
    
    When streaming a `Framework` into a log output, we commonly do not use surrounding ticks or alike.
    
    See e.g. https://github.com/apache/mesos/blob/868ec364b17bc231697563f2621705baff2d7183/src/master/master.cpp#L3075
    
    Here is the implementation of that operator; https://github.com/apache/mesos/blob/868ec364b17bc231697563f2621705baff2d7183/src/master/master.hpp#L2965-L2978
    
    And this is how that commonly looks in the end;
    ```
    I0503 01:55:54.630450 236896256 master.cpp:3200] Framework a0035e5a-14eb-4f6d-873e-e3cea144b682-0000 (default) at scheduler-ae407694-95bb-4188-90a9-07e55025c788@192.168.178.20:59261 failed over
    ```



src/master/http.cpp
Line 952 (original), 952-953 (patched)
<https://reviews.apache.org/r/66229/#comment284094>

    We commonly do not use multiple arguments in a single line on an argument continuation -- rather unfortunate btw cause that is what mesos-clang-format does in some cases. So if we would format like you suggest, then we would actually have to break every parameter into a new line.
    
    In this specific case breaking at the assignment, which is always preferred, actually solves it much nicer.
    
    ```
    Option<Error> error =
      validation::scheduler::call::validate(call, master, principal)
    ```



src/master/metrics.cpp
Lines 265 (patched)
<https://reviews.apache.org/r/66229/#comment284108>

    Thanks for this.


- Till Toenshoff


On April 20, 2018, 8:19 p.m., Kapil Arya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66229/
> -----------------------------------------------------------
> 
> (Updated April 20, 2018, 8:19 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Till Toenshoff.
> 
> 
> Bugs: MESOS-7258
>     https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented UPDATE_FRAMEWORK call.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp 135ae4389623385a6638158f5f23d6daca14a0ad 
>   src/master/master.hpp c30cf082d9ddf2e5dd97e0d887d6d07d8e03cbd4 
>   src/master/master.cpp c723a291ed8d219ced4492bc905ac6b52683ae47 
>   src/master/metrics.hpp ec76dbcd1d1fa5349d62ce73fb9603e1986a776b 
>   src/master/metrics.cpp e46ead79f3f29e285426f9d061337077f453aa45 
>   src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf 
>   src/master/validation.cpp ac2e1bb8771841ec59b3bdcdeffb6c6230680d4d 
> 
> 
> Diff: https://reviews.apache.org/r/66229/diff/8/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>