----------------------------------------------------------- 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 > >
