Re: Review Request 55359: Consolidate update of allocations in `updateAllocation()`.

[Jiang Yan Xu]

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


Ship it!




Ship It!

- Jiang Yan Xu


On Feb. 6, 2017, 9:02 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55359/
> ---
> 
> (Updated Feb. 6, 2017, 9:02 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 
> 56d6791baa64189523df668749f4a7ab67d6b363 
> 
> Diff: https://reviews.apache.org/r/55359/diff/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 55359: Consolidate update of allocations in `updateAllocation()`.

[Anindya Sinha]

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

(Updated Feb. 7, 2017, 5:02 a.m.)


Review request for mesos, Benjamin Mahler and Jiang Yan Xu.


Changes
---

Addressed comment and rebased.


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 (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
56d6791baa64189523df668749f4a7ab67d6b363 

Diff: https://reviews.apache.org/r/55359/diff/


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 55359: Consolidate update of allocations in `updateAllocation()`.

[Jiang Yan Xu]

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



LGTM. Just need to take another look after rebasing to ship it.


src/master/allocator/mesos/hierarchical.cpp (lines 682 - 683)


"contained" is ambiguous, I think it's better if we convey the idea of "at 
least one copy".

How about:

```
Ensure that offered resources contain at least one copy of the consumed 
shared resource (guaranteed by master validation).
```



src/master/allocator/mesos/hierarchical.cpp (lines 696 - 697)


So `additional` now encompasses multiple launch operations but it feels 
like it's still valuable to log the taskIds in this call for debugging?



src/master/allocator/mesos/hierarchical.cpp (lines 732 - 733)


So now we have two reasons:

```
The agent's total resources shouldn't contain:
1. The additionally allocated shared resources.
2. `AllocationInfo` as set in `updatedOfferedResources`.
```

Maybe spell them out like this?


- Jiang Yan Xu


On Feb. 3, 2017, 2:01 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55359/
> ---
> 
> (Updated Feb. 3, 2017, 2:01 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 
> 5f540569043df9d2bb75416c8c36bb4dd7bd68a1 
> 
> Diff: https://reviews.apache.org/r/55359/diff/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 55359: Consolidate update of allocations in `updateAllocation()`.

[Anindya Sinha]

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

(Updated Feb. 3, 2017, 10:01 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 (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
5f540569043df9d2bb75416c8c36bb4dd7bd68a1 

Diff: https://reviews.apache.org/r/55359/diff/


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 55359: Consolidate update of allocations in `updateAllocation()`.

[Anindya Sinha]

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

(Updated Feb. 1, 2017, 10:45 p.m.)


Review request for mesos, Benjamin Mahler and Jiang Yan Xu.


Changes
---

Rebased after batch allocation patch was merged.


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 (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
ffa07087f9ed8d28b99cc4cde7c739cfd7edb1e1 

Diff: https://reviews.apache.org/r/55359/diff/


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 55359: Consolidate update of allocations in `updateAllocation()`.

[Anindya Sinha]

> 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
> > 
> >
> > 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 _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
> > 
> >
> > `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
> > 
> >
> > 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& 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.cre

Re: Review Request 55359: Consolidate update of allocations in `updateAllocation()`.

[Anindya Sinha]

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

(Updated Jan. 31, 2017, 5:59 a.m.)


Review request for mesos, Benjamin Mahler and Jiang Yan Xu.


Changes
---

Addressed review comments.


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 (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
f471b6848bebae601a7a0509e9c6ad5eab4fa4a2 

Diff: https://reviews.apache.org/r/55359/diff/


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 55359: Consolidate update of allocations in `updateAllocation()`.

[Anindya Sinha]

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


Changes
---

Rebased.


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 (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
c2211be7458755aeb91ef078e4bfe92ac474044a 

Diff: https://reviews.apache.org/r/55359/diff/


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 55359: Consolidate update of allocations in `updateAllocation()`.

[Jiang Yan Xu]

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



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


src/master/allocator/mesos/hierarchical.cpp (lines 640 - 652)


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



src/master/allocator/mesos/hierarchical.cpp (line 645)


`original` is updated in every iteration?

Isn't original offered resources the variable `offeredResources`?



src/master/allocator/mesos/hierarchical.cpp (lines 697 - 699)


I see you've pulled this up so we don't have to get the same 
`frameworkAllocation` in every iteration.

However perhaps we can do it with just `offeredResources`:

```
foreach (const Resource& resource, additional) {
  CHECK(offeredResources.contains(resource));
}
```

Indeed, the rule is more precisely "we allow tasks to request additional 
allocations if the (original) offered resources already contain the shared 
resources", right?



src/master/allocator/mesos/hierarchical.cpp (lines 704 - 712)


This doesn't seem to need to go inside the `foreach (const 
Offer::Operation& operation, operations)` loop? The launch operation is a no-op 
with LAUNCH. If outside the loop, we can just do

```
// ... update allocation-related variables first.

// Update the agent total resources so they are consistent with the updated 
allocation. 
// We don't directly use `updatedOfferedResources` here because we don't 
count 
// additionally allocated resources in the agent total resources. (See 
comments in `updateSlaveTotal`.)
Try updatedTotal = slaves[slaveId].total.apply(operations);
CHECK_SOME(updatedTotal);
updateSlaveTotal(slaveId, updatedTotal.get());
```



src/master/allocator/mesos/hierarchical.cpp (lines 748 - 749)


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& 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.c

Review Request 55359: Consolidate update of allocations in `updateAllocation()`.

[Anindya Sinha]

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

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

Diff: https://reviews.apache.org/r/55359/diff/


Testing
---

Tests passed.


Thanks,

Anindya Sinha