mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anindya Sinha <anindya_si...@apple.com>
Subject Re: Review Request 42992: Support sharing of resources through reference counting of resources.
Date Tue, 02 Feb 2016 00:36:31 GMT


> On Jan. 30, 2016, 4:02 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 899
> > <https://reviews.apache.org/r/42992/diff/1/?file=1226444#file1226444line899>
> >
> >     Do we also need to check contain here?
> >     
> >     I think that the unsharable should be failed if the current resoures does not
contain the unshared resources.

No, since the passed Resource "that" needs to have share=true. If that is not the case, it
is not a shared resource and hence cannot be unshared. If "that" is a shared resource, then
it falls through to the call to `_getExistingConsumers(that)` where the logic for num_consumers==0
is done.


> On Jan. 30, 2016, 4:02 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 1134
> > <https://reviews.apache.org/r/42992/diff/1/?file=1226444#file1226444line1134>
> >
> >     Here not checking "if the volume exists" but "if the volume is a subset of current
resources"

In `destroyable()`, check for `contains()` is done for non shareable resources. For shareable
resources, we do a similar check in `_getExistingConsumers()` but extract the resource to
ensure its num_consumers==0.
I think it covers both scenarios. Am I missing something?


> On Jan. 30, 2016, 4:02 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, lines 1652-1654
> > <https://reviews.apache.org/r/42992/diff/1/?file=1226444#file1226444line1652>
> >
> >     I think that we already set the num consumers as 0 when initialize, so when
the logic can go here?
> >     
> >     What is the difference of {None} and {0} here?

{None} is when num_consumers is not set, and 0 is when num_consumers is set to 0. We do not
send or expect to receive num_consumers for shareable resources in Resources for all messages
going between master <-> framework. This condition handles the scenario when Resource
is logged at these points when num_consumers is not set. An example is when we log the offer
received while running all our test frameworks.


> On Jan. 30, 2016, 4:02 a.m., Guangya Liu wrote:
> > src/common/resources_utils.cpp, lines 71-73
> > <https://reviews.apache.org/r/42992/diff/1/?file=1226445#file1226445line71>
> >
> >     Do we need to move this under 68 under `isPersistentVolume` condition section?

We can but I think it is fine here as well, especially because shareable resources are supported
for all resource types but enabled for persistent volumes right now. Except the check in `Option<Error>
Resources::validate(const Resource& resource)`, I do not think there are any references
to assert that only persistent volumes can be shared.
Also in future, when we do extend sharing of resources to other resource tyoes as well, we
should be good to go here.


- Anindya


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


On Feb. 2, 2016, 12:36 a.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42992/
> -----------------------------------------------------------
> 
> (Updated Feb. 2, 2016, 12:36 a.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 194750e92020753e60154083a47bdc3398d31466 
>   include/mesos/resources.hpp 6bfac2e7e8799e74d87c7570fc5eef320ba76eb1 
>   include/mesos/v1/mesos.proto 1102bbc92f46f97c1915c03a71c7cf829003e0ed 
>   include/mesos/v1/resources.hpp 5a88c0756db2ea8db0f5df7ea3019b511ea135af 
>   src/common/resources.cpp 588a279c3cdebbeb58047bfbff5e78a42f53fc13 
>   src/common/resources_utils.cpp 70e6f025d89383084ab8b2cda23ab1cd55d959b2 
>   src/master/allocator/mesos/hierarchical.cpp 1a07d69016407e5aad2209586da37fecbcddb765

>   src/master/allocator/sorter/drf/sorter.cpp db47d640e36c0302d7c6254a9c58caa878feac01

>   src/master/master.hpp 3a7e18232323a1c051bcc97915484b1195fffe58 
>   src/master/master.cpp 98441a543611d4083b2495ee103f0ab5e2187e83 
>   src/master/validation.hpp 380b40279faf180a6f401a5e28280b601dbc648c 
>   src/master/validation.cpp f2bc1bad79e3b0812c019be3774cd65b58ea2d07 
>   src/v1/resources.cpp be4a5d153e9313cb71a6e85d1ed25a358537f2b7 
> 
> Diff: https://reviews.apache.org/r/42992/diff/
> 
> 
> Testing
> -------
> 
> make check done.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message