Re: Review Request 33865: Added RevocableInfo message to Resource protobuf.
On May 15, 2015, 6:09 p.m., Niklas Nielsen wrote: src/master/validation.cpp, line 90 https://reviews.apache.org/r/33865/diff/2/?file=960728#file960728line90 Do you want to stringify the resource for the error message here too? wanted to be consistent with the rest of the error messages here. i think we didn't originally stringify because it is always 'disk' resource. - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33865/#review83954 --- On May 14, 2015, 11:57 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33865/ --- (Updated May 14, 2015, 11:57 p.m.) Review request for mesos, Jie Yu, Joris Van Remoortere, and Niklas Nielsen. Bugs: MESOS-2691 https://issues.apache.org/jira/browse/MESOS-2691 Repository: mesos Description --- RevocableInfo currently supports OVERSUBSCRIBED resources. In the future it can be easily extended to use other types of revocable reosurces (e.g., resources allocated to other roles). Disabled the ability to use revocable resources for reservation or persistence because the semantics seem weird. We can enable it in the future if there is a use case for that. Diffs - include/mesos/mesos.proto 15f55a3c54be3475e77d561f106e00ea6e53c2fa src/common/resources.cpp 843a06d6c4d3e9ff0d1665360bae7c57bcfecb83 src/master/validation.cpp c3e96ae0e684f3f365e9aa365bccc953d32b0452 src/tests/resources_tests.cpp a7ec59ea217ad71f7d1e93ca6039d5b2491b3237 Diff: https://reviews.apache.org/r/33865/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 33865: Added RevocableInfo message to Resource protobuf.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33865/ --- (Updated May 15, 2015, 11:28 p.m.) Review request for mesos, Jie Yu, Joris Van Remoortere, and Niklas Nielsen. Changes --- jie's and nik's comments. PTAL. Bugs: MESOS-2691 https://issues.apache.org/jira/browse/MESOS-2691 Repository: mesos Description --- RevocableInfo currently supports OVERSUBSCRIBED resources. In the future it can be easily extended to use other types of revocable reosurces (e.g., resources allocated to other roles). Disabled the ability to use revocable resources for reservation or persistence because the semantics seem weird. We can enable it in the future if there is a use case for that. Diffs (updated) - include/mesos/mesos.proto 15f55a3c54be3475e77d561f106e00ea6e53c2fa src/common/resources.cpp 843a06d6c4d3e9ff0d1665360bae7c57bcfecb83 src/master/validation.cpp c3e96ae0e684f3f365e9aa365bccc953d32b0452 src/tests/master_validation_tests.cpp 9a6f4fa464d6daca088c8634e73e04c5fd380677 src/tests/resources_tests.cpp a7ec59ea217ad71f7d1e93ca6039d5b2491b3237 Diff: https://reviews.apache.org/r/33865/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 33865: Added RevocableInfo message to Resource protobuf.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33865/ --- (Updated May 14, 2015, 11:57 p.m.) Review request for mesos, Jie Yu, Joris Van Remoortere, and Niklas Nielsen. Changes --- jie's comments. Bugs: MESOS-2691 https://issues.apache.org/jira/browse/MESOS-2691 Repository: mesos Description --- RevocableInfo currently supports OVERSUBSCRIBED resources. In the future it can be easily extended to use other types of revocable reosurces (e.g., resources allocated to other roles). Disabled the ability to use revocable resources for reservation or persistence because the semantics seem weird. We can enable it in the future if there is a use case for that. Diffs (updated) - include/mesos/mesos.proto 15f55a3c54be3475e77d561f106e00ea6e53c2fa src/common/resources.cpp 843a06d6c4d3e9ff0d1665360bae7c57bcfecb83 src/master/validation.cpp c3e96ae0e684f3f365e9aa365bccc953d32b0452 src/tests/resources_tests.cpp a7ec59ea217ad71f7d1e93ca6039d5b2491b3237 Diff: https://reviews.apache.org/r/33865/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 33865: Added RevocableInfo message to Resource protobuf.
On May 7, 2015, 12:38 a.m., Jie Yu wrote: include/mesos/mesos.proto, lines 454-458 https://reviews.apache.org/r/33865/diff/1/?file=950607#file950607line454 Chatted with Vinod offline (PS: Vinod is going to send out a summary of the discussion). In short, OVERSUBSCRIBED = (Allocated but unused) + (Unallocated) So, the type `OVERSUBSCRIBED` here is too general and ambiguate. You description of this review is interesting. Resources reserved for a different role but unused (or unallocated) can be revokable. The question is to whom should we send this revokable resources. For instance, if a framework under role A reserved some resources on the slave (resources' role == A) and not using any of them yet. Should we send revokable offers to framework under role A, or all frameworks. It's not clear yet and I think that's a policy issue. It would be better to let a modulized component (e.g., resources estimator) to control that so that we can potentially use different policies. If we decide to send revokable offer for those reserved resources to all frameworks, we could strip the role in the revokable resources (i.e., make them * resources). However, I don't know what type should we set for the RevokableInfo for the above case? How can we distinguish that with the case where the resources are from unreserved resources (i.e., anyone can use that)? As a result, I think maybe we should punt on the `type` here right now and just leave an empty RevokableInfo since it's not strictly needed? Fair point. I will remove the type for now. On May 7, 2015, 12:38 a.m., Jie Yu wrote: include/mesos/mesos.proto, line 455 https://reviews.apache.org/r/33865/diff/1/?file=950607#file950607line455 2 spaces indent please. N/A On May 7, 2015, 12:38 a.m., Jie Yu wrote: src/common/resources.cpp, lines 490-501 https://reviews.apache.org/r/33865/diff/1/?file=950608#file950608line490 Should we put these checks to master validation? As I discussed with Mpark week ago, the validations in Resources object should be minimal checks to ensure all methods in Resources are well behaved. Clearly, the check you have here is a bit high level and probably should be put in master validation? Moved to validation.cpp. Also, I think the last check in Resource::validate() (* resource cannot be dynamically reserved) should be moved to master validation? - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33865/#review82763 --- On May 5, 2015, 9:13 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33865/ --- (Updated May 5, 2015, 9:13 p.m.) Review request for mesos, Jie Yu, Joris Van Remoortere, and Niklas Nielsen. Bugs: MESOS-2691 https://issues.apache.org/jira/browse/MESOS-2691 Repository: mesos Description --- RevocableInfo currently supports OVERSUBSCRIBED resources. In the future it can be easily extended to use other types of revocable reosurces (e.g., resources allocated to other roles). Disabled the ability to use revocable resources for reservation or persistence because the semantics seem weird. We can enable it in the future if there is a use case for that. Diffs - include/mesos/mesos.proto db4fc8c001dd68bc3b9ca83650170c4f26db18c7 src/common/resources.cpp 235930ff2dbb3ea49a3a0696dc070f2bd56fba4b src/tests/resources_tests.cpp a7ec59ea217ad71f7d1e93ca6039d5b2491b3237 Diff: https://reviews.apache.org/r/33865/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 33865: Added RevocableInfo message to Resource protobuf.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33865/#review82594 --- Patch looks great! Reviews applied: [33865] All tests passed. - Mesos ReviewBot On May 5, 2015, 9:13 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33865/ --- (Updated May 5, 2015, 9:13 p.m.) Review request for mesos, Jie Yu, Joris Van Remoortere, and Niklas Nielsen. Bugs: MESOS-2691 https://issues.apache.org/jira/browse/MESOS-2691 Repository: mesos Description --- RevocableInfo currently supports OVERSUBSCRIBED resources. In the future it can be easily extended to use other types of revocable reosurces (e.g., resources allocated to other roles). Disabled the ability to use revocable resources for reservation or persistence because the semantics seem weird. We can enable it in the future if there is a use case for that. Diffs - include/mesos/mesos.proto db4fc8c001dd68bc3b9ca83650170c4f26db18c7 src/common/resources.cpp 235930ff2dbb3ea49a3a0696dc070f2bd56fba4b src/tests/resources_tests.cpp a7ec59ea217ad71f7d1e93ca6039d5b2491b3237 Diff: https://reviews.apache.org/r/33865/diff/ Testing --- make check Thanks, Vinod Kone
Review Request 33865: Added RevocableInfo message to Resource protobuf.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33865/ --- Review request for mesos, Jie Yu, Joris Van Remoortere, and Niklas Nielsen. Bugs: MESOS-2691 https://issues.apache.org/jira/browse/MESOS-2691 Repository: mesos Description --- RevocableInfo currently supports OVERSUBSCRIBED resources. In the future it can be easily extended to use other types of revocable reosurces (e.g., resources allocated to other roles). Disabled the ability to use revocable resources for reservation or persistence because the semantics seem weird. We can enable it in the future if there is a use case for that. Diffs - include/mesos/mesos.proto db4fc8c001dd68bc3b9ca83650170c4f26db18c7 src/common/resources.cpp 235930ff2dbb3ea49a3a0696dc070f2bd56fba4b src/tests/resources_tests.cpp a7ec59ea217ad71f7d1e93ca6039d5b2491b3237 Diff: https://reviews.apache.org/r/33865/diff/ Testing --- make check Thanks, Vinod Kone
