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 60562: Updated `accept` to perform operation adjustment in one place.
Date Sat, 01 Jul 2017 05:44:24 GMT

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


Fix it, then Ship it!




Made some suggestions for cleanups, feel free to add TODOs and tackle them later in this chain.
Looks good from a correctness perspective.


src/master/master.cpp
Lines 3883-3886 (original), 3870-3873 (patched)
<https://reviews.apache.org/r/60562/#comment254150>

    I was about to make this exact suggestion to simplify this function, then studying the
code more carefully I noticed we already added a TODO :)



src/master/master.cpp
Lines 3929-3930 (patched)
<https://reviews.apache.org/r/60562/#comment254153>

    Any reason not to pull out the master specific normalization / upgrade of operations into
a utility function that calls out to the various pieces? Seems to be a logical normalization
/ upgrade on Operations that the master needs to perform without consulting any of the master
state?



src/master/master.cpp
Lines 3942-3947 (original), 4001-4005 (patched)
<https://reviews.apache.org/r/60562/#comment254152>

    I would suggest putting the generalized normalization at the top before the switch, so
that it's clear to the reader that there are general normalizations we apply before dealing
with the operation specific logic here. At a first read of the code, it looks like it's all
going to be within the switch.
    
    Ideally, all the normalization could be factored together, I left another comment about
that.



src/master/master.cpp
Lines 4738-4739 (original), 4764-4765 (patched)
<https://reviews.apache.org/r/60562/#comment254151>

    Any reason these validateAndUpgradeResources in `_accept()` are not part of the normalization?
    
    It's not obvious to me, maybe add a NOTE about how the normalization of operations isn't
doing this, above the normalization block you added in accept()?


- Benjamin Mahler


On June 30, 2017, 9:01 a.m., Michael Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60562/
> -----------------------------------------------------------
> 
> (Updated June 30, 2017, 9:01 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7735
>     https://issues.apache.org/jira/browse/MESOS-7735
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> It used to be that the minor adjustments that were made to operations
> were done in various places across `accept` and `_accept`.
> 
> The "executor-injection" for LAUNCH_GROUP was at the beginning of
> `accept`, "allocation-info-injection" for MULTI_ROLE was after offer
> validation, and "health-check-injection" for LAUNCH was in `_accept`.
> 
> The `Master::accept` function is now broken down into distinct
> "metrics accounting", "offer validation", "operation-adjustments", and
> "authorization" stages.
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp 276b18e4d1bab7e9b28c1c93fe13c93a38abc5cd 
> 
> 
> Diff: https://reviews.apache.org/r/60562/diff/1/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>


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