> On Jan. 26, 2017, 1:11 a.m., Jiang Yan Xu wrote: > > My feeling is that we were using too many variables for the same thing and > > after this review there are still more than we need. > > > > > > Feels like there should just be these variables to capture the allocation > > change: > > > > `offeredResources`: passed down from the master, this is the "original". > > `updatedOfferedResources`: the version with all the operations applied, > > either in `Resources::apply()`, or within this method for `LAUNCH`. This is > > the "updated", but more clear, since we are dealing with both total > > resources and allocations. > > > > Then we can update all the allocation related varaibles using these two. > > > > Then, next, we update the agent total resources. Why updating the totals in > > the method `updateAllocation()`? We can clarify this in the method (see my > > comments there).
As discussed, the flow in `updateAllocation()` is now as follows: ``` // (1) Make a copy of `offeredResources` (called `updatedOfferedResources`) to make changes // to original offered resources based on offer operations. // (2) Iterate thorugh each of the operations as follows: // (2a) For all operations, `updatedOfferedResources = updatedOfferedResources.apply()` // (2b) For LAUNCH, accumulate all of task resources for all tasks in `consumed`. // (3) Determine additional copies of shared resources to be added to allocations based // on `consumed` and `updatedOfferedResources`. // (4) Update the allocations in the allocator and the sorters. // (5) Update the total resources so that they are consistent with the updated allocation. ``` The main change is to process all operastions first and then update the allocations followed by total resources together (instead of doing it per operation). > On Jan. 26, 2017, 1:11 a.m., Jiang Yan Xu wrote: > > src/master/allocator/mesos/hierarchical.cpp, lines 640-652 > > <https://reviews.apache.org/r/55359/diff/1/?file=1600298#file1600298line640> > > > > To unify these variables, perhaps don't use `_offeredResources`. To be > > more consistent with the naming pattern within this method (`something` and > > `updatedSomething`), perhaps: > > > > ``` > > Resources updatedOfferedResources = offeredResources; > > > > // (The objective of this loop is to obtain the correct > > `updatedOfferedResources`.) > > foreach (const Offer::Operation& operation, operations) { > > if (operation.type() == Offer::Operation::LAUNCH) { > > // LAUNCH custom logic. > > // (Here everything we do is essentially also udpate > > `updatedOfferedResources`.) > > // ... > > } else { > > // (Here we invoke `Resources::apply()` to update > > `updatedOfferedResources`.) > > Try<Resources> _updatedOfferedResources = > > updatedOfferedResources.apply(operation); > > CHECK_SOME(_updatedOfferedResources); > > updatedOfferedResources = _updatedOfferedResources.get(); > > } > > } > > > > // At this point, we are outside the loop and we have "fully" processed > > the offered > > // resources to obtain `updatedOfferedResources`. > > // Now use it to update allocator and sorter variables. > > > > // ... > > ``` > > > > We don't need `updated` to capture the updated offered resources again. Addressed based on the top comment. > On Jan. 26, 2017, 1:11 a.m., Jiang Yan Xu wrote: > > src/master/allocator/mesos/hierarchical.cpp, line 645 > > <https://reviews.apache.org/r/55359/diff/1/?file=1600298#file1600298line645> > > > > `original` is updated in every iteration? > > > > Isn't original offered resources the variable `offeredResources`? The updated flow removes the need for `original` and `updated`. > On Jan. 26, 2017, 1:11 a.m., Jiang Yan Xu wrote: > > src/master/allocator/mesos/hierarchical.cpp, lines 786-787 > > <https://reviews.apache.org/r/55359/diff/1/?file=1600298#file1600298line786> > > > > After all operations perhaps it's valuable to CHECK that we indeed > > didn't change the quantities. So for this we can save the framework's > > allocation at the very beginning and compare with it at the very end. > > > > ``` > > void HierarchicalAllocatorProcess::updateAllocation( > > const FrameworkID& frameworkId, > > const SlaveID& slaveId, > > const Resources& offeredResources, > > const vector<Offer::Operation>& operations) > > { > > // ... some initial checks. > > > > // (Save `frameworkAllocation`.) > > const Resources frameworkAllocation = > > frameworkSorter->allocation(frameworkId.value(), slaveId); > > > > // Do the work. > > > > // Check that the quantities are not changed. > > const Resources updatedFrameworkAllocation = > > frameworkSorter->allocation(frameworkId.value(), slaveId); > > CHECK_EQ(frameworkAllocation.createStrippedScalarQuantity(), > > updatedFrameworkAllocation.createStrippedScalarQuantity()); > > > > LOG(INFO) << "Updated allocation of framework " << frameworkId > > << " on agent " << slaveId > > << " from " << frameworkAllocation > > << " to " << updatedFrameworkAllocation; > > } > > ``` We need to `flatten()` the `frameworkAllocation` and `updatedFrameworkAllocation` since the roles may change as a result of a `RESERVE` or `UNRESERVE` offer operations. - Anindya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55359/#review163031 ----------------------------------------------------------- On Jan. 26, 2017, 10:06 p.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55359/ > ----------------------------------------------------------- > > (Updated Jan. 26, 2017, 10:06 p.m.) > > > Review request for mesos, Benjamin Mahler and Jiang Yan Xu. > > > Bugs: MESOS-6444 > https://issues.apache.org/jira/browse/MESOS-6444 > > > Repository: mesos > > > Description > ------- > > We handle shared resources for `LAUNCH` operations separately in the > `updateAllocation()` API to add additional copies of shared resources. > But the updates to the allocations in the allocator and sorters > can be consolidated together. > > > Diffs > ----- > > src/master/allocator/mesos/hierarchical.cpp > c2211be7458755aeb91ef078e4bfe92ac474044a > > Diff: https://reviews.apache.org/r/55359/diff/ > > > Testing > ------- > > Tests passed. > > > Thanks, > > Anindya Sinha > >
