> On April 16, 2015, 7:27 p.m., Ben Mahler wrote:
> > src/master/master.hpp, line 1071
> > <https://reviews.apache.org/r/31665/diff/6/?file=931105#file931105line1071>
> >
> >     For example, could we document in NOTE on these total variables that 
> > subtraction is safe even though non-scalars lose information?
> >     
> >     Specifically, is my understanding correct here: 
> >     
> >     (1) For overlapping set items / ranges across slaves, these will get 
> > added N times but only represented 1 time in the Resources. 
> >     (2) So, when an initial subtraction occurs (N-1), the resource is no 
> > longer represented. We don't expose non-scalars for now, so this is safe 
> > for now.
> >     (3) When any further subtractions occur (N-(1+M)), the Resources simply 
> > ignores the subtraction since there's nothing to remove, so this is safe 
> > for now.

I think this was meant as a subsequent comment to the one above, so I'll answer 
here.

I'm not sure if I correctly understand your definition of __safety__ and what's 
considered a __bug__.
> (1) For overlapping set items / ranges across slaves, these will get added N 
> times but only represented 1 time in the Resources.

Correct.
> (2) So, when an initial subtraction occurs (N-1), the resource is no longer 
> represented.

Correct.
> We don't expose non-scalars for now, so this is safe for now.

We actually do expose non-scalars through the master HTTP endpoint. We used to 
only expose `cpus`, `mem`, `disk`, and `ports` but since 
[r31082](https://reviews.apache.org/r/31082) we expose all resources. But even 
before, `ports` was reported incorrectly. If we mean __safe__ as in we don't 
crash, then yes it's safe. But I would probably consider this to be at least a 
__bug__.
> (3) When any further subtractions occur (N-(1+M)), the Resources simply 
> ignores the subtraction since there's nothing to remove, so this is safe for 
> now.

Correct.

After the sequence of patches land, the totals (used and offered) are merely 
used to be exposed via `state.json`. The non-scalars are still incorrect, but 
I'm not sure that we can assume that nobody's using them. If we decide to only 
expose the scalar resources since the non-scalar information was wrong anyway, 
I think that should definitely be done in a separate patch at least to keep the 
commits clean (as I mentioned, MESOS-2623 will track that). Furthermore, I 
think changing what resources get exposed via `state.json` is considered an API 
change and therefore is probably worthwhile mentioning in the `CHANGELOG` for 
the next release.


- Michael


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


On April 15, 2015, 10:45 p.m., Michael Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31665/
> -----------------------------------------------------------
> 
> (Updated April 15, 2015, 10:45 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-2373
>     https://issues.apache.org/jira/browse/MESOS-2373
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> `master::Framework` holds 2 member variables of type `Resources`: 
> `usedResources` and `offeredResources`. Both of these are aggregates of 
> resources from multiple slaves. We add the `hashmap<SlaveID, Resources>` 
> versions since we can lose non-scalar resources by summing them up across 
> multiple slaves. For further details refer to 
> [MESOS-2373](https://issues.apache.org/jira/browse/MESOS-2373).
> 
> In [r31666](https://reviews.apache.org/r/31666/), we report 
> `usedResourcesBySlaveId` rather than `usedResources` when adding the 
> framework to the allocator.
> 
> We don't actually use `offeredResourcesBySlaveId` currently, but it should be 
> kept for symmetry. There will be a ticket to expose `usedResourcesBySlaveId` 
> as well as `offeredResourcesBySlaveId` via the HTTP endpoint.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp 6141917644b84edfed9836fa0a005d55a36880e3 
> 
> Diff: https://reviews.apache.org/r/31665/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Reply via email to