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 55461: Made resource reservation validation multi-role aware.
Date Mon, 06 Feb 2017 21:28:41 GMT

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




src/master/master.cpp (lines 3942 - 3947)
<https://reviews.apache.org/r/55461/#comment236112>

    This isn't quite correct since we need to check the capability to determine which field
to examine.
    
    Per my suggestion below, can we pass the FrameworkInfo in rather than just the roles?
    
    Also note that there is a `getRoles` helper in protobuf utils to simplify this. We should
add a `set<string> roles` field to the `Framework` struct in the master though.



src/master/validation.hpp (lines 223 - 227)
<https://reviews.apache.org/r/55461/#comment236110>

    Hm.. any reason this shouldn't follow the pattern used in the create validation right
below where we take framework info? That seems to make it a bit clearer that the optional
framework means it's none when done by operator.
    
    E.g.
    
    ```
    // Validates the RESERVE operation.
    Option<Error> validate(
        const Offer::Operation::Reserve& reserve,
        const Option<std::string>& principal,
        const Option<FrameworkInfo>& frameworkInfo = None());
    ```



src/master/validation.cpp (lines 1500 - 1503)
<https://reviews.apache.org/r/55461/#comment236113>

    Hm.. do you know which call sites need to be updated? It seems like we need to do the
sweep now to ensure the call-sites are correct, no?



src/master/validation.cpp (lines 1509 - 1513)
<https://reviews.apache.org/r/55461/#comment236115>

    This condition isn't quite the `*` role, this would be a framework with no roles. The
`*` role case is now the 1 element `*` set, but it seems to me this check should be looking
at the resource.role() rather than the framework's roles, no?


- Benjamin Mahler


On Jan. 16, 2017, 12:27 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55461/
> -----------------------------------------------------------
> 
> (Updated Jan. 16, 2017, 12:27 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jay Guo, and Guangya Liu.
> 
> 
> Bugs: MESOS-6730
>     https://issues.apache.org/jira/browse/MESOS-6730
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This updates the resource reservation validation for frameworks which
> can have multiple roles. During a deprecation period 'FrameworkInfo'
> will have fields for both 'role' and 'roles', however the validation
> function works with just an optional set of roles. Here an empty set
> captures the previous semantics of either having an empty 'role' field
> or 'role' set as '*'. This forces the callers to properly construct a
> set of framework roles from the available information. An optional set
> is used in order to accommodate callers which have no information
> about the framework's roles, and ultimately disables validation taking
> that information into account.
> 
> 
> Diffs
> -----
> 
>   src/master/master.cpp b863ff6e93931c3d1ee056248084c7f44caf2fd9 
>   src/master/validation.hpp 57e81779ff7444904c2ad7bad33aaf9167b98d05 
>   src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f 
>   src/tests/master_validation_tests.cpp e5d55e03648cb218d42adc594d6fa7d40ea9bcbb 
> 
> Diff: https://reviews.apache.org/r/55461/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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