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 55462: Validate resource reservation against allocated role.
Date Tue, 14 Feb 2017 00:11:28 GMT

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




src/master/validation.cpp (lines 1617 - 1629)
<https://reviews.apache.org/r/55462/#comment237238>

    We should be able to just remove these per the discussion from the last review.



src/master/validation.cpp (line 1668)
<https://reviews.apache.org/r/55462/#comment237242>

    s/+s/+ s/



src/master/validation.cpp (lines 1668 - 1670)
<https://reviews.apache.org/r/55462/#comment237243>

    Hm.. the ", but" line seems a bit odd, how about:
    
    ```
            return Error(
                "A reserved operation was attempted on unallocated resource"
                " '" +stringify(resource) + "', but frameworks can only"
                " perform reservations on allocated resources");
    ```



src/master/validation.cpp (lines 1675 - 1677)
<https://reviews.apache.org/r/55462/#comment237244>

    Is it possible to get the quotes on the same line? E.g.
    
    ```
            return Error(
                "A reserve operation was attempted for a resource with role"
                " '" + resource.role() + "', but the resource was allocated"
                " to role '" + resource.allocation_info().role() + "'");
    ```



src/master/validation.cpp (lines 1682 - 1686)
<https://reviews.apache.org/r/55462/#comment237237>

    Is it possible to put the quotes on the same line?
    
    ```
            return Error(
                "A reserve operation was attempted for a resource allocated to role"
                " '" + resource.role() + "', but the framework only has roles"
                " '" + stringify(frameworkRoles.get()) + "'");
    ```



src/master/validation.cpp (line 1694)
<https://reviews.apache.org/r/55462/#comment237245>

    s/"' "/" '"/



src/master/validation.cpp (lines 1698 - 1699)
<https://reviews.apache.org/r/55462/#comment237239>

    For a bit more clarity: "set because operators can only reserve the available resources"



src/master/validation.cpp (lines 1702 - 1703)
<https://reviews.apache.org/r/55462/#comment237246>

    Hm.. what does "out-of-band" allocation info mean?
    
    How about something like the following, which makes it clear what the operator API allows:
    
    ```
    A reserve operation was attempted with an allocated resource but the operator API only
allows reservations
    to be made to the unallocated resources
    ```



src/tests/master_validation_tests.cpp (lines 424 - 425)
<https://reviews.apache.org/r/55462/#comment237247>

    How about having this block right next to the reserve block since these two seem related,
seems a bit clearer?



src/tests/master_validation_tests.cpp (lines 471 - 490)
<https://reviews.apache.org/r/55462/#comment237248>

    We could generalize this test to be: a framework (that has role `"*"` in its roles, e.g.
`{role1, role2, *}`) cannot make a reservation to `*`. As it stands we seem to be missing
the case where star is present but there are multiple roles.



src/tests/master_validation_tests.cpp (lines 493 - 513)
<https://reviews.apache.org/r/55462/#comment237250>

    This could be generalized to be making a reservation to a role that is not within its
roles (which may be already caught by the mismatched allocation role since we expect allocation
role == reserve role)?


- Benjamin Mahler


On Feb. 13, 2017, 3:38 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55462/
> -----------------------------------------------------------
> 
> (Updated Feb. 13, 2017, 3:38 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 change introduces validation of the 'AllocationInfo' of resources
> used in reservations.
> 
> 
> Diffs
> -----
> 
>   src/master/validation.cpp 2bfca95713319a195e43e990cbc2dc5570b89c4e 
>   src/tests/master_validation_tests.cpp fd1f4a6cf0661351e265e50da1bd6ea04ed13d26 
> 
> Diff: https://reviews.apache.org/r/55462/diff/
> 
> 
> Testing
> -------
> 
> N/A yet.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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