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 72955: Consolidated creation and validation of `allocator::Framework` options.
Date Thu, 15 Oct 2020 19:27:14 GMT

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


Ship it!





src/master/master.hpp
Lines 1061-1062 (original), 1061-1062 (patched)
<https://reviews.apache.org/r/72955/#comment311125>

    Perhaps:
    
    ```
    Performs stateless and stateful validation of the FrameworkInfo.
    The stateful validation only uses the master flags and whether
    the framework is completed.
    
    TODO: Consider passing in the state explicitly to move this
    function into validation.hpp for unit testability.
    ```



src/master/master.cpp
Lines 2615-2616 (patched)
<https://reviews.apache.org/r/72955/#comment311126>

    It seems unfortunate that validatation of the suppressed roles is buried within creating
the allocator framework options, which leads to it being modeled as a "failure to create the
options" rather than a "validation error". It's less clear if the failure is due to invalidness
or some kind of error on our part.
    
    The explicit approach makes it clear:
    
    ```
    Option<Error> e = validateFramework(info);
    
    ...
    
    // Validate the other fields of the subscription:
    e = validateSuppressedRoles(info, suppressedRoles);
    ...
    e = validateOfferConstraints(info, offerConstraints);
    
    make allocator options w/ hard failure if not valid
    ```
    
    Also, by containing all of the API validation within our validation headers we can unit
test all of it, and for integration tests we only need to test that the functions are called
(rather than testing all invalid cases via the integration tests). That was the idea when
we started to contain it all within validation headers.
    
    Just the thought I had while reading this, not suggesting that you necessarily change
this.



src/master/validation.hpp
Lines 133-135 (patched)
<https://reviews.apache.org/r/72955/#comment311127>

    Per the above comment, the nice part of our stateless validators is we can unit test them
(if you want to add a quick one to this patch), and we only need to do the heavy integration
test for the wiring.



src/master/validation.cpp
Lines 692-693 (patched)
<https://reviews.apache.org/r/72955/#comment311124>

    don't know if we need the quotes here, since we're not showing a string field:
    
    `Suppressed roles {"foo", "bar"} are not ...`


- Benjamin Mahler


On Oct. 14, 2020, 6:16 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72955/
> -----------------------------------------------------------
> 
> (Updated Oct. 14, 2020, 6:16 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-10176
>     https://issues.apache.org/jira/browse/MESOS-10176
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This merges three near-identical pieces of scattered code in SUBSCRIBE
> and UPDATE_FRAMEWORK execution paths in the Master that validate
> and construct parts of `allocator::FrameworkOptions` (the set of
> suppressed roles and the offer constraints filter) into a single
> function.
> 
> This is a prerequisite to adding validation of offer constraint roles.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 83d8190702ff25b5e3e9607d8250aa331aaad028 
>   src/master/master.cpp d6d3ea7e27539a65b601783eca2ac4e3b688f188 
>   src/master/validation.hpp 7fe8f087b322f8e77218f4dd9f9f75707afa3208 
>   src/master/validation.cpp 5b1bcb5207aac0553bf9c1a0d89525c835393ccb 
> 
> 
> Diff: https://reviews.apache.org/r/72955/diff/3/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>


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