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 60021: Updated the use of `toUnreserved()`.
Date Wed, 14 Jun 2017 18:13:07 GMT

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


Fix it, then Ship it!




Looks good, although this merits a description, how about:

```
Updated (UN)RESERVE operation application to handle reservation refinement.

The logic for applying RESERVE operations now needs to ensure that the
pre-reserved resources are being refined. In other words, the RESERVE
operation should be "pushing" a single reservation to the stack. Note
that we only support "pushing" a single reservation at a time.

For UNRESERVE, the operation should "pop" the last reservation off the
stack. We only support "popping" a single reservation at a time.
```


src/common/resources.cpp
Line 1474 (original), 1474 (patched)
<https://reviews.apache.org/r/60021/#comment251565>

    Can you add a note here saying we only allow "pushing" a single reservation at a time
to the "stack" of reservations?
    
    I noted we should document this under [MESOS-7663](https://issues.apache.org/jira/browse/MESOS-7663).



src/common/resources.cpp
Line 1505 (original), 1505 (patched)
<https://reviews.apache.org/r/60021/#comment251566>

    Ditto here, can you add a note here saying we only allow "popping" a single reservation
at a time to the "stack" of reservations?



src/master/http.cpp
Lines 2245-2248 (original), 2245-2248 (patched)
<https://reviews.apache.org/r/60021/#comment251564>

    Do you want to re-write this note to reflect that we only allow the operator to push one
reservation at a time?
    
    Also, not yours, but reading this code I'm left confused as to why we need to pass the
popped resources of the operation and the operation into the continuation. I think the following
would clear both of these up?
    
    ```
    // We only allow pushing a single reservation at a time, so
    // we require the resources with one reservation "popped" to
    // be present on the agent.
    Resources required = resources.popReservation();
    
    return _operation(slaveId, required, operation);
    ```



src/master/http.cpp
Lines 2245-2249 (original), 2245-2250 (patched)
<https://reviews.apache.org/r/60021/#comment251638>

    Shouldn't I see a similar change in `Master::Http::_unreserve` logic? Striked me as a
little odd that they both didn't need a change.



src/master/http.cpp
Lines 2250 (patched)
<https://reviews.apache.org/r/60021/#comment251637>

    Why did you need to wrap with Resources() here? Did you originally have popReservation
as a mutating fucntion and this is a leftover?


- Benjamin Mahler


On June 13, 2017, 8:37 a.m., Michael Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60021/
> -----------------------------------------------------------
> 
> (Updated June 13, 2017, 8:37 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7655
>     https://issues.apache.org/jira/browse/MESOS-7655
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Updated the use of `toUnreserved()`.
> 
> 
> Diffs
> -----
> 
>   src/common/resources.cpp 7b3fae1a34150964ba3831a3aef2f868e338aef8 
>   src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323 
>   src/v1/resources.cpp 236fe17730918479a33314e51d14f3cc1679d432 
> 
> 
> Diff: https://reviews.apache.org/r/60021/diff/3/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Michael Park
> 
>


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