----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53096/#review162890 -----------------------------------------------------------
Fix it, then Ship it! This review is now closely related to the followup review /r/55359/ which almost makes sense to combine them. But because this patch fixes a bug (with tests to prove that) I guess we'll still keep them separate. Just thought I'd mention it for posterity. src/master/allocator/mesos/hierarchical.hpp (line 479) <https://reviews.apache.org/r/53096/#comment234272> As mentioned in the reply from the previous conversation: s/updateTotal/updateSlaveTotal/ And the comments: ``` // Helper to update the agent's total resources maintained in the allocator and the role // and quota sorters (whose total resources match the agent's total resources). ``` src/master/allocator/mesos/hierarchical.cpp (line 720) <https://reviews.apache.org/r/53096/#comment234480> s/total/agent's total/ src/master/allocator/mesos/hierarchical.cpp (line 754) <https://reviews.apache.org/r/53096/#comment234481> s/allocated resources/allocation/ It's synonymous but `allocation` is more consistent with the comments above (now that we don't need to say "total and allocated resources"). Same applies to another occurrence below. src/master/allocator/mesos/hierarchical.cpp (line 1977) <https://reviews.apache.org/r/53096/#comment234492> Can we also add the following sentense before "So, we update them using the resources in `slaves[slaveId].total`." to explain it more thoroughly: ``` (which don't get updated in the allocation runs or during recovery of allocated resources) ``` src/tests/persistent_volume_tests.cpp (lines 1443 - 1447) <https://reviews.apache.org/r/53096/#comment234495> "We kill the long-lived task": this duplicates the same "We kill the long-lived task" below. I guess you mentioned them in two places because they are far apart but can we move ``` Future<TaskStatus> status4; EXPECT_CALL(sched, statusUpdate(&driver, _)) .WillOnce(FutureArg<1>(&status4)); ``` down to right before we sent killTask so we just need to comment on it once? Plus, the block of code below it right now is all about killing the `nonSharedVolume` which is not relevant. - Jiang Yan Xu On Jan. 9, 2017, 2:37 p.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53096/ > ----------------------------------------------------------- > > (Updated Jan. 9, 2017, 2:37 p.m.) > > > Review request for mesos and Jiang Yan Xu. > > > Bugs: MESOS-6444 > https://issues.apache.org/jira/browse/MESOS-6444 > > > Repository: mesos > > > Description > ------- > > We maintain a single copy of shared resource in the role and quota > sorter's total resources. So, when we update these resources, we > remove the previous resources at this agent and add the new resources > at this agent. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.hpp > a6424d624864155e1c87a28a63b784512c5c8722 > src/master/allocator/mesos/hierarchical.cpp > 91b1ec43940a788459f045ca4a4b82d4e8373bca > src/tests/persistent_volume_tests.cpp > 8198b6b5ad323d17835ba067c7ff3d34ef948125 > > Diff: https://reviews.apache.org/r/53096/diff/ > > > Testing > ------- > > Tests passed. > > > Thanks, > > Anindya Sinha > >
