Re: Review Request 55828: Updated Resources::apply to handle Resource.AllocationInfo.

[Benjamin Mahler]

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



Filed https://issues.apache.org/jira/browse/MESOS-7048 to move the stripping of 
allocation info up into the call-sites rather than putting it inside 
`Resources::apply()`.

- Benjamin Mahler


On Jan. 23, 2017, 10:47 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55828/
> ---
> 
> (Updated Jan. 23, 2017, 10:47 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-6967
> https://issues.apache.org/jira/browse/MESOS-6967
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, `Resource` did not contain `AllocationInfo`. So for
> backwards compatibility with old schedulers and tooling, we must
> allow operations to contain `Resource`s without an allocation role.
> The two interesting cases for adjusting the operation's resource are:
> 
> (1) The operation `Resource` does not contain an `AllocationInfo`
> but is being applied to an allocated `Resources`. We allow this
> only if the operation is unambiguous, that is, the allocated
> `Resources` are only allocated to a single role.
> 
> (2) The operation `Resource` contains an `AllocationInfo` but is
> being applied to an unallocated `Resources`. In this case we
> simply ignore the `AllocationInfo` of the `Resource`.
> 
> Note that we assume no `Resources` store a mix of allocated and
> unallocated resources. This is a brittle assumption that we should
> have enforcement for.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45 
>   src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68 
>   src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32 
>   src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267 
> 
> Diff: https://reviews.apache.org/r/55828/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55828: Updated Resources::apply to handle Resource.AllocationInfo.

[Benjamin Mahler]

> On Jan. 28, 2017, 3:35 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, lines 1274-1276
> > 
> >
> > This indicates that if the `Resource` do not have `AllocationInfo`, 
> > then we will loop all of the `Resource` till finished then return false, 
> > but we have the assumption of no `Resources` store a mix of allocated and 
> > unallocated resources, so how about `return 
> > resource.has_allocation_info();` here?

I was a bit hestitant to say it's not allocated if one but not all of the 
resources have allocation info. I agree though that we need to enforce the 
invariant or have separate types for allocated and unallocated resources to 
prevent mixing, at which point we can write the more efficient version.


- Benjamin


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


On Jan. 23, 2017, 10:47 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55828/
> ---
> 
> (Updated Jan. 23, 2017, 10:47 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-6967
> https://issues.apache.org/jira/browse/MESOS-6967
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, `Resource` did not contain `AllocationInfo`. So for
> backwards compatibility with old schedulers and tooling, we must
> allow operations to contain `Resource`s without an allocation role.
> The two interesting cases for adjusting the operation's resource are:
> 
> (1) The operation `Resource` does not contain an `AllocationInfo`
> but is being applied to an allocated `Resources`. We allow this
> only if the operation is unambiguous, that is, the allocated
> `Resources` are only allocated to a single role.
> 
> (2) The operation `Resource` contains an `AllocationInfo` but is
> being applied to an unallocated `Resources`. In this case we
> simply ignore the `AllocationInfo` of the `Resource`.
> 
> Note that we assume no `Resources` store a mix of allocated and
> unallocated resources. This is a brittle assumption that we should
> have enforcement for.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45 
>   src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68 
>   src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32 
>   src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267 
> 
> Diff: https://reviews.apache.org/r/55828/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55828: Updated Resources::apply to handle Resource.AllocationInfo.

[Benjamin Mahler]

> On Jan. 29, 2017, 8:02 a.m., Michael Park wrote:
> > src/common/resources.cpp, lines 1257-1261
> > 
> >
> > We talked about how this condition probably isn't necessary since we 
> > set the `AllocationInfo` in
> > `Offer::Operation`s in the master (and redundantly in the allocator).
> > 
> > Please let me know how that turned out!

I can remove this one since we do the injection at the call-sites. For now I'll 
leave in the second case but put a TODO to consider call-sites performing the 
stripping of allocation info when necessary.


> On Jan. 29, 2017, 8:02 a.m., Michael Park wrote:
> > src/common/resources.cpp, line 1339
> > 
> >
> > Shouldn't we print `unreserved` as opposed to `adjustedReservation` 
> > here?

If we were to print the pre-adjusted resource the error seems weird because it 
will never contain the resource. I think we'll move towards removing adjustment 
entirely from `Resources::apply()` and having call-site adjust (either inject 
or strip). I put a TODO for this.


- Benjamin


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


On Jan. 23, 2017, 10:47 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55828/
> ---
> 
> (Updated Jan. 23, 2017, 10:47 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-6967
> https://issues.apache.org/jira/browse/MESOS-6967
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, `Resource` did not contain `AllocationInfo`. So for
> backwards compatibility with old schedulers and tooling, we must
> allow operations to contain `Resource`s without an allocation role.
> The two interesting cases for adjusting the operation's resource are:
> 
> (1) The operation `Resource` does not contain an `AllocationInfo`
> but is being applied to an allocated `Resources`. We allow this
> only if the operation is unambiguous, that is, the allocated
> `Resources` are only allocated to a single role.
> 
> (2) The operation `Resource` contains an `AllocationInfo` but is
> being applied to an unallocated `Resources`. In this case we
> simply ignore the `AllocationInfo` of the `Resource`.
> 
> Note that we assume no `Resources` store a mix of allocated and
> unallocated resources. This is a brittle assumption that we should
> have enforcement for.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45 
>   src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68 
>   src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32 
>   src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267 
> 
> Diff: https://reviews.apache.org/r/55828/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55828: Updated Resources::apply to handle Resource.AllocationInfo.

[Benjamin Mahler]

> On Jan. 30, 2017, 4:40 p.m., Benjamin Bannier wrote:
> > src/common/resources.cpp, line 1288
> > 
> >
> > This looks like an expensive computation. Should we precompute it at 
> > function scope and capture it here by ref?
> 
> Benjamin Mahler wrote:
> Agreed, the issue here is that the current implementation of 
> allocations() CHECK fails if the resources are unallocated, so calling it 
> blindly will crash.
> For now I'll put a TODO.

Actually, this is no longer needed.


- Benjamin


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


On Jan. 23, 2017, 10:47 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55828/
> ---
> 
> (Updated Jan. 23, 2017, 10:47 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-6967
> https://issues.apache.org/jira/browse/MESOS-6967
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, `Resource` did not contain `AllocationInfo`. So for
> backwards compatibility with old schedulers and tooling, we must
> allow operations to contain `Resource`s without an allocation role.
> The two interesting cases for adjusting the operation's resource are:
> 
> (1) The operation `Resource` does not contain an `AllocationInfo`
> but is being applied to an allocated `Resources`. We allow this
> only if the operation is unambiguous, that is, the allocated
> `Resources` are only allocated to a single role.
> 
> (2) The operation `Resource` contains an `AllocationInfo` but is
> being applied to an unallocated `Resources`. In this case we
> simply ignore the `AllocationInfo` of the `Resource`.
> 
> Note that we assume no `Resources` store a mix of allocated and
> unallocated resources. This is a brittle assumption that we should
> have enforcement for.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45 
>   src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68 
>   src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32 
>   src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267 
> 
> Diff: https://reviews.apache.org/r/55828/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55828: Updated Resources::apply to handle Resource.AllocationInfo.

[Benjamin Mahler]

> On Jan. 30, 2017, 4:40 p.m., Benjamin Bannier wrote:
> > src/common/resources.cpp, lines 1263-1266
> > 
> >
> > Could you clarify under what conditions this can happen?
> > 
> > It seems surprising to silently drop information explicitly given by 
> > users, and I believe rejecting the operation would be clearer. If 
> > situations can occur where a user has more recent information than the 
> > current master (e.g., an allocation info not yet known to the master) we 
> > should fix failover behavior.

There are two intake paths for operations:

(1) When a framework accepts an offer, and
(2) When an operator uses an endpoint to apply an operation (e.g. reserve, 
create volume, etc).

In case (1) we have to apply the operation to the allocated resources. This 
will fail if the allocation info is incorrect, and if it's absent we will 
inject it for them prior to applying it (this at least needs to be done for 
old-style frameworks, but it is done for MULTI_ROLE frameworks as well since it 
seemed like an easier upgrade path for schedulers since they don't need to 
ensure they carry or set the allocation info into the operations when they 
accept). However, we then need to update our internal state for the agent's 
total and allocated resources. The former has no allocation info and the latter 
has a variety of allocation infos. Because we have to apply the same operation 
to both, we either need to handle this within `Resources::apply()` or each call 
site needs to strip the allocation info prior to applying to the total. I went 
with the former approach because it seemed easier on the callers, but given the 
confusion here, perhaps it is clearer to impose the stripping on the
  callers. I'll add a TODO. Make sense?

In case (2) this doesn't occur.


> On Jan. 30, 2017, 4:40 p.m., Benjamin Bannier wrote:
> > src/common/resources.cpp, line 1288
> > 
> >
> > This looks like an expensive computation. Should we precompute it at 
> > function scope and capture it here by ref?

Agreed, the issue here is that the current implementation of allocations() 
CHECK fails if the resources are unallocated, so calling it blindly will crash.
For now I'll put a TODO.


> On Jan. 30, 2017, 4:40 p.m., Benjamin Bannier wrote:
> > src/tests/resources_utils.hpp, line 40
> > 
> >
> > This class seems confusing. It e.g., is not a wrapper since when 
> > constructing an `AllocatedResources` from some unallocated `Resources` an 
> > assertion like  `EXPECT_EQ(resources, allocated)` is not true (i.e., it 
> > doesn't just wrap some `Resources` and some role).
> > 
> > What about making this a function for now, e.g.,
> > 
> > Resources allocatedResources(
> > const Resources& resources,
> > const string& role);

I'm not sure I followed the point about the EXPECT_EQ, but the intention was to 
signal to the reader that we may want a type for our flavors of resoures 
(allocated vs unallocated) in order to prevent invalid operations and the 
mixing of unallocated and allocated resources. I'll make it a function for now 
since we don't have a use for the class.


> On Jan. 30, 2017, 4:40 p.m., Benjamin Bannier wrote:
> > src/common/resources.cpp, line 1272
> > 
> >
> > Did you intend to make this `const`? If not, the following appears more 
> > straight-forward to me,
> > 
> > bool isAllocated = false;
> > foreach(const Resource& resource, resources) {
> >   if (resource.has_allocation_info()) {
> > isAllocated = true;
> > break;
> >   }
> > }
> > 
> > If you'd went for the optimization suggested by Guangya this does 
> > become a "one-liner" though,
> > 
> > const bool isAllocated =
> >   resources.resources().empty()
> > ? false
> > : resources.resources(0).has_allocation_info();

Yeah, all of these sound fine to me, however I was looking to signal to the 
reader that isAllocated is a function applied to Resources. If we had better 
enforcement of the invariant that there is no mixing of resources then the 
optimization would sound good to me.

I'll make it const.


> On Jan. 30, 2017, 4:40 p.m., Benjamin Bannier wrote:
> > src/common/resources.cpp, lines 1268-1270
> > 
> >
> > `..., see MESOS-6636.` to allow better tracking of the status of this.

Hm.. MESOS-6636 is about enforcing that executors and tasks do not mix 
resources within themselves, which will be enforced at the validation layer. 
What I'm saying here is that our `Resources` class itself doesn't 

Re: Review Request 55828: Updated Resources::apply to handle Resource.AllocationInfo.

[Benjamin Bannier]

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




src/common/resources.cpp (lines 1263 - 1266)


Could you clarify under what conditions this can happen?

It seems surprising to silently drop information explicitly given by users, 
and I believe rejecting the operation would be clearer. If situations can occur 
where a user has more recent information than the current master (e.g., an 
allocation info not yet known to the master) we should fix failover behavior.



src/common/resources.cpp (lines 1268 - 1270)


`..., see MESOS-6636.` to allow better tracking of the status of this.



src/common/resources.cpp (line 1272)


Did you intend to make this `const`? If not, the following appears more 
straight-forward to me,

bool isAllocated = false;
foreach(const Resource& resource, resources) {
  if (resource.has_allocation_info()) {
isAllocated = true;
break;
  }
}

If you'd went for the optimization suggested by Guangya this does become a 
"one-liner" though,

const bool isAllocated =
  resources.resources().empty()
? false
: resources.resources(0).has_allocation_info();



src/common/resources.cpp (line 1288)


This looks like an expensive computation. Should we precompute it at 
function scope and capture it here by ref?



src/tests/resources_utils.hpp (line 40)


This class seems confusing. It e.g., is not a wrapper since when 
constructing an `AllocatedResources` from some unallocated `Resources` an 
assertion like  `EXPECT_EQ(resources, allocated)` is not true (i.e., it doesn't 
just wrap some `Resources` and some role).

What about making this a function for now, e.g.,

Resources allocatedResources(
const Resources& resources,
const string& role);


- Benjamin Bannier


On Jan. 23, 2017, 11:47 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55828/
> ---
> 
> (Updated Jan. 23, 2017, 11:47 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-6967
> https://issues.apache.org/jira/browse/MESOS-6967
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, `Resource` did not contain `AllocationInfo`. So for
> backwards compatibility with old schedulers and tooling, we must
> allow operations to contain `Resource`s without an allocation role.
> The two interesting cases for adjusting the operation's resource are:
> 
> (1) The operation `Resource` does not contain an `AllocationInfo`
> but is being applied to an allocated `Resources`. We allow this
> only if the operation is unambiguous, that is, the allocated
> `Resources` are only allocated to a single role.
> 
> (2) The operation `Resource` contains an `AllocationInfo` but is
> being applied to an unallocated `Resources`. In this case we
> simply ignore the `AllocationInfo` of the `Resource`.
> 
> Note that we assume no `Resources` store a mix of allocated and
> unallocated resources. This is a brittle assumption that we should
> have enforcement for.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45 
>   src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68 
>   src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32 
>   src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267 
> 
> Diff: https://reviews.apache.org/r/55828/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55828: Updated Resources::apply to handle Resource.AllocationInfo.

[Michael Park]

> On Jan. 29, 2017, 12:02 a.m., Michael Park wrote:
> > It took me a while trying to figure out what the invariants of `Resources` 
> > and `Offer::Operation` are, but I think I have a grasp of it.
> > 
> > - We have `recovered.apply(operation);` where we have `recovered` and 
> > `operation` both with unallocated resources. (after `recovered` adjustment)
> > - We have a few `offeredResources.apply(operation);` where we have 
> > `offeredResources` and `operation` both with resources allocated to a 
> > single role. (after `operation` adjustment)
> > - We also have `totalResources.apply(operation);` where we have 
> > `totalResources` with unallocated resources, and `operation` with resources 
> > allocated to a single role. (case 2 helps us with this)
> > 
> > It seems to me like we really end up with resources allocated to 
> > __multiple__ roles if we were to allow accepting multiple offers from 
> > multiple roles.
> > That is, currently the resources in `Resources` and `Offer::Operation` are 
> > either all unallocated, or all allocated to one role.
> > 
> > Is this accurate?

Ah, I was wrong about `Resources`. It can hold resources that are allocated to 
multiple roles.
I was correct about `Offer::Operation`, however. In that they can only be 
unallocated or allocated to one role.


- Michael


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


On Jan. 23, 2017, 2:47 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55828/
> ---
> 
> (Updated Jan. 23, 2017, 2:47 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-6967
> https://issues.apache.org/jira/browse/MESOS-6967
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, `Resource` did not contain `AllocationInfo`. So for
> backwards compatibility with old schedulers and tooling, we must
> allow operations to contain `Resource`s without an allocation role.
> The two interesting cases for adjusting the operation's resource are:
> 
> (1) The operation `Resource` does not contain an `AllocationInfo`
> but is being applied to an allocated `Resources`. We allow this
> only if the operation is unambiguous, that is, the allocated
> `Resources` are only allocated to a single role.
> 
> (2) The operation `Resource` contains an `AllocationInfo` but is
> being applied to an unallocated `Resources`. In this case we
> simply ignore the `AllocationInfo` of the `Resource`.
> 
> Note that we assume no `Resources` store a mix of allocated and
> unallocated resources. This is a brittle assumption that we should
> have enforcement for.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45 
>   src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68 
>   src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32 
>   src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267 
> 
> Diff: https://reviews.apache.org/r/55828/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55828: Updated Resources::apply to handle Resource.AllocationInfo.

[Michael Park]

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


Fix it, then Ship it!




It took me a while trying to figure out what the invariants of `Resources` and 
`Offer::Operation` are, but I think I have a grasp of it.

- We have `recovered.apply(operation);` where we have `recovered` and 
`operation` both with unallocated resources. (after `recovered` adjustment)
- We have a few `offeredResources.apply(operation);` where we have 
`offeredResources` and `operation` both with resources allocated to a single 
role. (after `operation` adjustment)
- We also have `totalResources.apply(operation);` where we have 
`totalResources` with unallocated resources, and `operation` with resources 
allocated to a single role. (case 2 helps us with this)

It seems to me like we really end up with resources allocated to __multiple__ 
roles if we were to allow accepting multiple offers from multiple roles.
That is, currently the resources in `Resources` and `Offer::Operation` are 
either all unallocated, or all allocated to one role.

Is this accurate?


src/common/resources.cpp (lines 1257 - 1261)


We talked about how this condition probably isn't necessary since we set 
the `AllocationInfo` in
`Offer::Operation`s in the master (and redundantly in the allocator).

Please let me know how that turned out!



src/common/resources.cpp (line 1339)


Shouldn't we print `unreserved` as opposed to `adjustedReservation` here?


- Michael Park


On Jan. 23, 2017, 2:47 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55828/
> ---
> 
> (Updated Jan. 23, 2017, 2:47 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-6967
> https://issues.apache.org/jira/browse/MESOS-6967
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, `Resource` did not contain `AllocationInfo`. So for
> backwards compatibility with old schedulers and tooling, we must
> allow operations to contain `Resource`s without an allocation role.
> The two interesting cases for adjusting the operation's resource are:
> 
> (1) The operation `Resource` does not contain an `AllocationInfo`
> but is being applied to an allocated `Resources`. We allow this
> only if the operation is unambiguous, that is, the allocated
> `Resources` are only allocated to a single role.
> 
> (2) The operation `Resource` contains an `AllocationInfo` but is
> being applied to an unallocated `Resources`. In this case we
> simply ignore the `AllocationInfo` of the `Resource`.
> 
> Note that we assume no `Resources` store a mix of allocated and
> unallocated resources. This is a brittle assumption that we should
> have enforcement for.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45 
>   src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68 
>   src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32 
>   src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267 
> 
> Diff: https://reviews.apache.org/r/55828/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55828: Updated Resources::apply to handle Resource.AllocationInfo.

[Guangya Liu]

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


Fix it, then Ship it!




Ship It!


src/common/resources.cpp (lines 1274 - 1276)


This indicates that if the `Resource` do not have `AllocationInfo`, then we 
will loop all of the `Resource` till finished then return false, but we have 
the assumption of no `Resources` store a mix of allocated and unallocated 
resources, so how about `return resource.has_allocation_info();` here?



src/tests/resources_utils.hpp (line 27)


I think that we do not like `using xxx` in a header file.



src/tests/resources_utils.hpp (line 33)


I think that we can kill the keyword `TODO` here?


- Guangya Liu


On 一月 23, 2017, 10:47 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55828/
> ---
> 
> (Updated 一月 23, 2017, 10:47 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-6967
> https://issues.apache.org/jira/browse/MESOS-6967
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, `Resource` did not contain `AllocationInfo`. So for
> backwards compatibility with old schedulers and tooling, we must
> allow operations to contain `Resource`s without an allocation role.
> The two interesting cases for adjusting the operation's resource are:
> 
> (1) The operation `Resource` does not contain an `AllocationInfo`
> but is being applied to an allocated `Resources`. We allow this
> only if the operation is unambiguous, that is, the allocated
> `Resources` are only allocated to a single role.
> 
> (2) The operation `Resource` contains an `AllocationInfo` but is
> being applied to an unallocated `Resources`. In this case we
> simply ignore the `AllocationInfo` of the `Resource`.
> 
> Note that we assume no `Resources` store a mix of allocated and
> unallocated resources. This is a brittle assumption that we should
> have enforcement for.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45 
>   src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68 
>   src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32 
>   src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267 
> 
> Diff: https://reviews.apache.org/r/55828/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>