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