mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call.
Date Sun, 12 May 2019 05:46:45 GMT

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




src/master/master.hpp
Lines 665-668 (patched)
<https://reviews.apache.org/r/70533/#comment301786>

    No need to re-iterate all these details here, we're unlikely to keep it in sync with what
the code does.



src/master/master.hpp
Lines 674 (patched)
<https://reviews.apache.org/r/70533/#comment301789>

    s/make/Make/



src/master/master.hpp
Lines 676 (patched)
<https://reviews.apache.org/r/70533/#comment301788>

    s/is/it/



src/master/master.hpp
Lines 677-678 (patched)
<https://reviews.apache.org/r/70533/#comment301787>

    The early newline makes it look like this last sentence is potentially separate from the
TODO when reading this, how about just continuing from the previous line?
    
    ```
      // currently it is not, see MESOS-9746. At that point, we can
      // potentially remove the `broadcastFrameworkUpdate()` function.
    ```



src/master/master.hpp
Lines 685 (patched)
<https://reviews.apache.org/r/70533/#comment301780>

    The arguments are generally named in these declarations, we should stay consistent with
that



src/master/master.hpp
Lines 1073-1079 (patched)
<https://reviews.apache.org/r/70533/#comment301779>

    The arguments are generally named in the other declarations, we should stay consistent
with that



src/master/master.cpp
Lines 2877-2880 (original), 2884-2887 (patched)
<https://reviews.apache.org/r/70533/#comment301778>

    Hm.. why doesn't the broadcast function send it to subscribers too? why the need to have
this separately done outside the function?
    
    Probably want to rename it if we're able to do both in the function: `broadcastFramwerkUpdate(...)`



src/master/master.cpp
Lines 2886-2894 (original), 2901-2909 (patched)
<https://reviews.apache.org/r/70533/#comment301777>

    Seems like we can construct the message outside the loop? Feel free to follow up with
a patch later for that or have a patch in front of this patch.



src/master/master.cpp
Lines 3279-3280 (patched)
<https://reviews.apache.org/r/70533/#comment301785>

    This.. seems odd. Do we need to rename this function now that it's more a master-stateful
validation of the framework info?
    
    Reading this code, I'm left thinking that the validation of the old vs new FrameworkInfo
is accidentally omitted, but I'm guessing your previous patches actually put that within the
`validateFrameworkSubscription(...)` function and that's how it's getting validated here?
    
    Seems like we need to do some renaming here, it seems to be more a stateful validation
of the framework info now, not subscription.



src/master/master.cpp
Lines 3287-3291 (patched)
<https://reviews.apache.org/r/70533/#comment301783>

    Seems unfortunate that this is getting caught here instead of within validation of the
Call. It would be nice if this function can just CHECK it.



src/master/master.cpp
Lines 3309 (patched)
<https://reviews.apache.org/r/70533/#comment301790>

    Start all TODOs with a capital letter, you should find that's the norm if you grep the
codebase



src/master/master.cpp
Lines 3327 (patched)
<https://reviews.apache.org/r/70533/#comment301784>

    Can we be more clear here?
    
    Are we referring to concurrent SUBSCRIBE or UPDATE_FRAMEWORK calls?
    
    It seems to me we're relying on authorization preserving the ordering



src/master/master.cpp
Lines 3338 (patched)
<https://reviews.apache.org/r/70533/#comment301781>

    no double newline here



src/master/master.cpp
Lines 3345 (patched)
<https://reviews.apache.org/r/70533/#comment301782>

    newline


- Benjamin Mahler


On May 7, 2019, 1:09 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70533/
> -----------------------------------------------------------
> 
> (Updated May 7, 2019, 1:09 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
>     https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch is based on the previous implementation attempt:
> https://reviews.apache.org/r/66229/
> 
> 
> Diffs
> -----
> 
>   src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 35a04a3d0183697f93dadb3cbfead3ee0c2fea08

>   src/master/http.cpp 765bbf1442f26dcc08c5404f714b6a2ef6616ed8 
>   src/master/master.hpp 7d9732f1e432f6f0290d234242864cbdbf381fa8 
>   src/master/master.cpp a8ee6297e1587c160a47b806914d3c3aa4f35cd7 
>   src/master/validation.cpp 9fb0850987ce385d345302cac9721adead7181b8 
> 
> 
> Diff: https://reviews.apache.org/r/70533/diff/5/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>


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