> On Feb. 13, 2016, 12:57 a.m., Greg Mann wrote:
> > include/mesos/resources.hpp, line 222
> > <https://reviews.apache.org/r/42992/diff/3/?file=1236753#file1236753line222>
> >
> >     I might recommend renaming this method, due to the ubiquitous `get()` 
> > method of the `Future` and `Option` types. Perhaps `find()` instead?

There is a method `Option<Resources> find(const Resources& targets) const;` 
which does different from what this function does. This method extracts the 
Resource object from Resources that matches the passed in Resource& by not 
comparing the num_consumers.
So, let us rename this to:
`Option<Resource> locate(const Resource& that) const;`

Does it sound ok to you?

> On Feb. 13, 2016, 12:57 a.m., Greg Mann wrote:
> > include/mesos/mesos.proto, lines 988-989
> > <https://reviews.apache.org/r/42992/diff/3/?file=1236752#file1236752line988>
> >
> >     I'm curious if we'll really need these operations? Might it be enough 
> > to simply make a persistent volume shareable when it's created? The same 
> > might be the case for other shareable resources as well.

I think that supporting SHARE as add-on operations for resources that can be 
shared (but are not created as shared) is useful. Same argument for UNSHARE.
Keeping in mind that this feature is for sharing resources (not just sharing of 
persistent volumes), I feel that these 2 apis have a need to co-exist.

> On Feb. 13, 2016, 12:57 a.m., Greg Mann wrote:
> > include/mesos/mesos.proto, line 688
> > <https://reviews.apache.org/r/42992/diff/3/?file=1236752#file1236752line688>
> >
> >     I don't think this field belongs in the protobuf. If this information 
> > doesn't need to be communicated on the wire, the master can simply track it 
> > internally, and reconstruct it upon agent reregistration after master 
> > failover.

As stated in the design doc, this has been done to make implementation simpler. 
Resource is used quite extensively in code and hence it was easier to embed 
num_consumers in it.
I agree we can keep num_consumers as a hashmap, such as:
`hashmap<Resource, uint32_t> consumers;` and expose it in a singleton or 
something like that such that multiple classes can have access to it, esp. the 
sorter, allocator and master.
However, the num_consumers vary based on its context. For example:
i) For role, it contains the total tasks within a specific role that master has 
assigned this resource. This is across frameworks.
ii) For framework, it contains the total tasks within a specific framework that 
master has assigned this resource.
iii) For slave, it contains the total tasks within a slave that master has 
assigned this resource (this should be same as (i) for persistent volumes).
So we would need to maintain multiple hashmaps to reflect this. Secondly, there 
needs to be a `hashmap::find` for every access to num_consumers which can be 
avoided if we embed it within Resource::ShareInfo.

Another point that has been raised is if we should reconsider and actually 
expose `num_consumerss` to frameworks since that would give a good idea for 
frameworks to know when a `DESTROY` can be issued. Note that the `DESTROY` is 
not guaranteed to succeed though since another framework of the same role might 
have issued a launch task using this resource in the mantime (and that is the 
reason we did not expose this in the first place). However, I think we can 
consider exposing this to frameworks if we feel that would provide frameworks 
with some meaningful information regarding possibility of DESTROY being 
successful. Note that the same issue exists with non-shareable persistent 
volumes as well, but the use case is not as bad since there can be only 1 task 
that can use the persistent volume.

- Anindya

This is an automatically generated e-mail. To reply, visit:

On Feb. 5, 2016, 10:57 p.m., Anindya Sinha wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42992/
> -----------------------------------------------------------
> (Updated Feb. 5, 2016, 10:57 p.m.)
> Review request for mesos and Adam B.
> Bugs: MESOS-4431
>     https://issues.apache.org/jira/browse/MESOS-4431
> Repository: mesos
> Description
> -------
> Added new Offer::Operation of SHARE and UNSHARE for resources.
> Added ShareInfo within Resources protobuf to allow for sharing of resources
> and keep track of consumers of such resources.
> Shareable resources keeps track of its consumers and makes decision based on
> that during task launch and task termination.
> Allow DESTROY or UNSHARE for shared volumes only if reference count is 0.
> Diffs
> -----
>   include/mesos/mesos.proto 5d1a20b4ff647f43701a353472b56a4b74b6bbc3 
>   include/mesos/resources.hpp 6bfac2e7e8799e74d87c7570fc5eef320ba76eb1 
>   include/mesos/v1/mesos.proto 415a20d9f3385e9627635dd89d78c58d03e3c116 
>   include/mesos/v1/resources.hpp 5a88c0756db2ea8db0f5df7ea3019b511ea135af 
>   src/common/resources.cpp 588a279c3cdebbeb58047bfbff5e78a42f53fc13 
>   src/common/resources_utils.cpp 70e6f025d89383084ab8b2cda23ab1cd55d959b2 
>   src/master/allocator/mesos/hierarchical.cpp 
> a9d2c23162892e22220f97d89a076d2311091d91 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 18797e42a9c2bd20392020237cfae600a5ffe12c 
>   src/master/master.hpp 3a7e18232323a1c051bcc97915484b1195fffe58 
>   src/master/master.cpp e1ca81dab85a7ab1391eca0d6bd995548bf79c22 
>   src/master/validation.hpp 380b40279faf180a6f401a5e28280b601dbc648c 
>   src/master/validation.cpp 66898e914c7b4ab83c4580be67530f355cfb05ca 
>   src/v1/resources.cpp be4a5d153e9313cb71a6e85d1ed25a358537f2b7 
> Diff: https://reviews.apache.org/r/42992/diff/
> Testing
> -------
> make check done.
> Thanks,
> Anindya Sinha

Reply via email to