Re: Review Request 42992: [1 of 7] Support sharing of resources through reference counting of resources.

2016-01-31 Thread Guangya Liu 

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




src/master/master.hpp (lines 127 - 138)


add some comments here;

if the shared resources is new added, then add it to totalResources; If the 
shared resources was already used in totalResources, just increase the 
reference by num consumers.



src/master/master.hpp (lines 163 - 176)


Ditto, add more comments here.



src/master/master.hpp (line 282)


Is it an old bug? I think that we should make sure that the task is in 
terminate state here?



src/master/master.hpp (line 1865)


Same question as above, is it a bug?



src/master/master.cpp (line 3700)


Even though this should never happen, we still should add more detailed 
info here, such as offer, shared resource info in the log message here.



src/master/master.cpp (line 3745)


remove the //



src/master/master.cpp (line 3764)


add some log here to tell how many resoruces are returning to allocator?



src/master/master.cpp (lines 5444 - 5445)


move this comments to 5452


- Guangya Liu


On 一月 30, 2016, 12:26 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42992/
> ---
> 
> (Updated 一月 30, 2016, 12:26 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.
> * 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
> 
>

Re: Review Request 42992: [1 of 7] Support sharing of resources through reference counting of resources.

2016-01-29 Thread Guangya Liu 

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



I think that you also need to update all of the summary of the patch chain to 
make sure it starts with a capital letter, otherwise, you will not able to 
apply your patch.


src/common/resources.cpp (lines 733 - 737)


Can you please add more accurate error message for this?

Such as 

 if (resource.name() != "disk") {
   return Error(
  "ShareInfo should not be set for " + resource.name() + " resource");
 }
 
 if (!resource.has_disk() || !resource.disk().has_persistence()) {
   return Error(
 "Only persistent volumes can be shared");
 }



src/common/resources.cpp (line 899)


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.



src/common/resources.cpp (line 1134)


Here not checking "if the volume exists" but "if the volume is a subset of 
current resources"



src/common/resources.cpp (lines 1652 - 1654)


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?



src/common/resources_utils.cpp (lines 71 - 73)


Do we need to move this under 68 under `isPersistentVolume` condition 
section?



src/master/master.cpp (line 3220)


Remove @


- Guangya Liu


On 一月 30, 2016, 12:26 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42992/
> ---
> 
> (Updated 一月 30, 2016, 12:26 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.
> * 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
> 
>

Re: Review Request 42992: [1 of 7] Support sharing of resources through reference counting of resources.

2016-01-29 Thread Guangya Liu 

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



Just some early comments, I was that most of your comments do not have a period 
at the end, it is better to fix this in all of your patches in this chain.


include/mesos/mesos.proto (line 679)


s/that/this?



include/mesos/mesos.proto (line 683)


add a period to the end



include/mesos/resources.hpp (line 221)


add a period to the end



include/mesos/resources.hpp (line 228)


I think that here we should stress that this function is mainly

"Checks if this Resources is eligible to be unshared"

What about removing "Resources contains the given Resource" from the 
comments as this logic should be implicit done by this function.



include/mesos/resources.hpp (line 230)


add a period to the end

s/0 consumers/no consumer.



include/mesos/resources.hpp (line 256)


add a period to the end



include/mesos/resources.hpp (line 399)


add a period to the end



src/common/resources.cpp (line 731)


add a period


- Guangya Liu


On 一月 30, 2016, 12:26 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42992/
> ---
> 
> (Updated 一月 30, 2016, 12:26 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.
> * 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
> 
>