You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2015/05/05 23:13:24 UTC

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

Re: Review Request 33865: Added RevocableInfo message to Resource protobuf.

Posted by Vinod Kone <vi...@gmail.com>.

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

Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33865/#review82763
-----------------------------------------------------------



include/mesos/mesos.proto
<https://reviews.apache.org/r/33865/#comment133596>

    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?



include/mesos/mesos.proto
<https://reviews.apache.org/r/33865/#comment133576>

    2 spaces indent please.



src/common/resources.cpp
<https://reviews.apache.org/r/33865/#comment133597>

    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?


- Jie Yu


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.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
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
> 
>

Re: Review Request 33865: Added RevocableInfo message to Resource protobuf.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33865/#review83880
-----------------------------------------------------------


Patch looks great!

Reviews applied: [33865]

All tests passed.

- Mesos ReviewBot


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.

Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33865/#review84005
-----------------------------------------------------------


Patch looks great!

Reviews applied: [34298, 33865]

All tests passed.

- Mesos ReviewBot


On May 15, 2015, 11:28 p.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> 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.
> 
> 
> 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/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.

Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33865/#review84002
-----------------------------------------------------------

Ship it!



src/master/validation.cpp
<https://reviews.apache.org/r/33865/#comment135104>

    Please use isDynamicallyReserved(resource)



src/master/validation.cpp
<https://reviews.apache.org/r/33865/#comment135103>

    Maybe add a predicte function in Resources?
    
    ```
    static bool Resources::isRevokable(resource);
    ```


- Jie Yu


On May 15, 2015, 11:28 p.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> 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.
> 
> 
> 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/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.

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
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.

Posted by Vinod Kone <vi...@gmail.com>.

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

Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33865/#review83954
-----------------------------------------------------------

Ship it!



src/master/validation.cpp
<https://reviews.apache.org/r/33865/#comment135011>

    Do you want to stringify the resource for the error message here too?



src/tests/resources_tests.cpp
<https://reviews.apache.org/r/33865/#comment135010>

    Had to read the containment tests twice; maybe introduce the tests with a small comment on what you are testing?
    
    I see that you grabbed the tests from above, but would still be helpful with an intro.


- Niklas Nielsen


On May 14, 2015, 4: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, 4: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.

Posted by Vinod Kone <vi...@gmail.com>.

> On May 15, 2015, 7:18 p.m., Jie Yu wrote:
> > src/master/validation.cpp, lines 89-91
> > <https://reviews.apache.org/r/33865/diff/2/?file=960728#file960728line89>
> >
> >     Revocable resources shouldn't be used with Persistent volume. We might want to support temp volume in the future and I think revocable resources are compatible with that.
> >     
> >     So either add a TODO here, or move the check under `if (resource.disk().has_persistence()` block.

moved the check into persistence.


- Vinod


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


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.

Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33865/#review83962
-----------------------------------------------------------

Ship it!



src/master/validation.cpp
<https://reviews.apache.org/r/33865/#comment135024>

    Do you want to add tests for that (and DiskInfo) in master_validation_tests.cpp (ResourceValidationTest)?



src/master/validation.cpp
<https://reviews.apache.org/r/33865/#comment135022>

    Revocable resources shouldn't be used with Persistent volume. We might want to support temp volume in the future and I think revocable resources are compatible with that.
    
    So either add a TODO here, or move the check under `if (resource.disk().has_persistence()` block.


- Jie Yu


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.

Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
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