Re: Review Request 35947: Added a new API call 'updateAvailable' to the allocator.
On July 8, 2015, 5:59 p.m., Alexander Rukletsov wrote: include/mesos/master/allocator.hpp, lines 133-135 https://reviews.apache.org/r/35947/diff/1/?file=993649#file993649line133 And we introduce a libprocess dependency into `Allocator` interface. I think it's a prominent step, right now an allocator implementation is not even required to be asynchronous. I don't want to say it's a bad change, I want us to think a bit more about the consequences of such step. Maybe it is worth discussing on the dev list. Jie Yu wrote: Most of our module interfaces depends on libprocess (e.g., resource estimator, qos controller, isolator, etc.). Why do you think this is a prominent step? right now an allocator implementation is not even required to be asynchronous I think at the time we introduced modules, we agreed on that internal interfaces are subject to change without needing a formal deprecation cycle. It's up to the module developer to use proper versioning to deal with changes in the interfaces. Alexander Rukletsov wrote: To clarify, I think it's fine updating an interface, but giving the growing complexity of allocator, introducing a libprocess dependency doesn't make it easier : ). However, if we all share same concerns and are eager to refactor the allocator interface in the near future, I'm fine with the change. Michael Park wrote: Hey Alex, I've filed [MESOS-3147](https://issues.apache.org/jira/browse/MESOS-3147) to keep track of the allocator refactor work and to get take the discussion out of these reviews. Please augment the ticket with your thoughts and plans, Thanks! That's great Michael, thank you very much! - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35947/#review90936 --- On July 24, 2015, 9:26 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35947/ --- (Updated July 24, 2015, 9:26 p.m.) Review request for mesos, Alexander Rukletsov, Benjamin Hindman, Ben Mahler, and Jie Yu. Bugs: MESOS-3146 https://issues.apache.org/jira/browse/MESOS-3146 Repository: mesos Description --- Needed to implement the master HTTP endpoints: `/reserve`, `/unreserve`, `/create` and `/destroy`. This API is similar to `updateSlave` which is currently very specific to oversubscription. I considered consolidating `updateAvailable` and `updateSlave` but it will require making offers be generated within the allocator to enable this. In specific, `updateAvailable` could fail if there aren't sufficient available resources to update, whereas `updateSlave` avoids failing by keeping the allocator in an over-allocated state. For `updateSlave`, leaving the allocator in an over-allocated state is ok. This is because it does not modify resources therefore `total - allocated` will work out to __empty__. `updateAvailable` cannot leave the allocator in an over-allocated state, because it modifies resources, and therefore `total - allocated` is not guaranteed to yield __empty__. Diffs - include/mesos/master/allocator.hpp 22992c0c77058af4fcd28aa8e4a1191693a16f44 src/master/allocator/mesos/allocator.hpp 72470ec7f56f84a9a9815c09adb88def90ef672f src/master/allocator/mesos/hierarchical.hpp 3264d145d52b48852878abf7ab9be29ab98208cc src/tests/hierarchical_allocator_tests.cpp 3258840135290cd008ca09235d18b7f093dafd2e src/tests/mesos.hpp 69134e1c2664ca24a1ecd80a662c841311104a6a Diff: https://reviews.apache.org/r/35947/diff/ Testing --- (1) Added `HierarchicalAllocatorTest.updateAvailableSuccess` and `HierarchicalAllocatorTest.updateAvailableFail` (2) `make check` Thanks, Michael Park
Re: Review Request 35947: Added a new API call 'updateAvailable' to the allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35947/ --- (Updated July 24, 2015, 3:50 p.m.) Review request for mesos, Alexander Rukletsov, Benjamin Hindman, Ben Mahler, and Jie Yu. Changes --- Addressed comments from Jie and Till. Repository: mesos Description --- Needed to implement the master HTTP endpoints: `/reserve`, `/unreserve`, `/create` and `/destroy`. This API is similar to `updateSlave` which is currently very specific to oversubscription. I considered consolidating `updateAvailable` and `updateSlave` but it will require making offers be generated within the allocator to enable this. In specific, `updateAvailable` could fail if there aren't sufficient available resources to update, whereas `updateSlave` avoids failing by keeping the allocator in an over-allocated state. For `updateSlave`, leaving the allocator in an over-allocated state is ok. This is because it does not modify resources therefore `total - allocated` will work out to __empty__. `updateAvailable` cannot leave the allocator in an over-allocated state, because it modifies resources, and therefore `total - allocated` is not guaranteed to yield __empty__. Diffs (updated) - include/mesos/master/allocator.hpp 22992c0c77058af4fcd28aa8e4a1191693a16f44 src/master/allocator/mesos/allocator.hpp 72470ec7f56f84a9a9815c09adb88def90ef672f src/master/allocator/mesos/hierarchical.hpp 3264d145d52b48852878abf7ab9be29ab98208cc src/tests/hierarchical_allocator_tests.cpp 3258840135290cd008ca09235d18b7f093dafd2e src/tests/mesos.hpp 69134e1c2664ca24a1ecd80a662c841311104a6a Diff: https://reviews.apache.org/r/35947/diff/ Testing --- (1) Added `HierarchicalAllocatorTest.updateAvailableSuccess` and `HierarchicalAllocatorTest.updateAvailableFail` (2) `make check` Thanks, Michael Park
Re: Review Request 35947: Added a new API call 'updateAvailable' to the allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35947/#review91081 --- src/tests/hierarchical_allocator_tests.cpp (line 785) https://reviews.apache.org/r/35947/#comment144350 Would be great if you could add a brief description as a comment to these tests as done for most others here as well. src/tests/hierarchical_allocator_tests.cpp (line 828) https://reviews.apache.org/r/35947/#comment144351 see above. - Till Toenshoff On June 26, 2015, 10:55 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35947/ --- (Updated June 26, 2015, 10:55 p.m.) Review request for mesos, Alexander Rukletsov, Benjamin Hindman, Ben Mahler, and Jie Yu. Repository: mesos Description --- Needed to implement the master HTTP endpoints: `/reserve`, `/unreserve`, `/create` and `/destroy`. This API is similar to `updateSlave` which is currently very specific to oversubscription. I considered consolidating `updateAvailable` and `updateSlave` but it will require making offers be generated within the allocator to enable this. In specific, `updateAvailable` could fail if there aren't sufficient available resources to update, whereas `updateSlave` avoids failing by keeping the allocator in an over-allocated state. For `updateSlave`, leaving the allocator in an over-allocated state is ok. This is because it does not modify resources therefore `total - allocated` will work out to __empty__. `updateAvailable` cannot leave the allocator in an over-allocated state, because it modifies resources, and therefore `total - allocated` is not guaranteed to yield __empty__. Diffs - include/mesos/master/allocator.hpp 22992c0c77058af4fcd28aa8e4a1191693a16f44 src/master/allocator/mesos/allocator.hpp 72470ec7f56f84a9a9815c09adb88def90ef672f src/master/allocator/mesos/hierarchical.hpp 3264d145d52b48852878abf7ab9be29ab98208cc src/tests/hierarchical_allocator_tests.cpp 3258840135290cd008ca09235d18b7f093dafd2e src/tests/mesos.hpp 9157ac079808d2686592e54ea26a26e6a0825ed3 Diff: https://reviews.apache.org/r/35947/diff/ Testing --- (1) Added `HierarchicalAllocatorTest.updateAvailableSuccess` and `HierarchicalAllocatorTest.updateAvailableFail` (2) `make check` Thanks, Michael Park
Re: Review Request 35947: Added a new API call 'updateAvailable' to the allocator.
On July 8, 2015, 5:59 p.m., Alexander Rukletsov wrote: include/mesos/master/allocator.hpp, lines 133-135 https://reviews.apache.org/r/35947/diff/1/?file=993649#file993649line133 And we introduce a libprocess dependency into `Allocator` interface. I think it's a prominent step, right now an allocator implementation is not even required to be asynchronous. I don't want to say it's a bad change, I want us to think a bit more about the consequences of such step. Maybe it is worth discussing on the dev list. Jie Yu wrote: Most of our module interfaces depends on libprocess (e.g., resource estimator, qos controller, isolator, etc.). Why do you think this is a prominent step? right now an allocator implementation is not even required to be asynchronous I think at the time we introduced modules, we agreed on that internal interfaces are subject to change without needing a formal deprecation cycle. It's up to the module developer to use proper versioning to deal with changes in the interfaces. To clarify, I think it's fine updating an interface, but giving the growing complexity of allocator, introducing a libprocess dependency doesn't make it easier : ). However, if we all share same concerns and are eager to refactor the allocator interface in the near future, I'm fine with the change. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35947/#review90936 --- On June 26, 2015, 10:55 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35947/ --- (Updated June 26, 2015, 10:55 p.m.) Review request for mesos, Alexander Rukletsov, Benjamin Hindman, Ben Mahler, and Jie Yu. Repository: mesos Description --- Needed to implement the master HTTP endpoints: `/reserve`, `/unreserve`, `/create` and `/destroy`. This API is similar to `updateSlave` which is currently very specific to oversubscription. I considered consolidating `updateAvailable` and `updateSlave` but it will require making offers be generated within the allocator to enable this. In specific, `updateAvailable` could fail if there aren't sufficient available resources to update, whereas `updateSlave` avoids failing by keeping the allocator in an over-allocated state. For `updateSlave`, leaving the allocator in an over-allocated state is ok. This is because it does not modify resources therefore `total - allocated` will work out to __empty__. `updateAvailable` cannot leave the allocator in an over-allocated state, because it modifies resources, and therefore `total - allocated` is not guaranteed to yield __empty__. Diffs - include/mesos/master/allocator.hpp 22992c0c77058af4fcd28aa8e4a1191693a16f44 src/master/allocator/mesos/allocator.hpp 72470ec7f56f84a9a9815c09adb88def90ef672f src/master/allocator/mesos/hierarchical.hpp 3264d145d52b48852878abf7ab9be29ab98208cc src/tests/hierarchical_allocator_tests.cpp 3258840135290cd008ca09235d18b7f093dafd2e src/tests/mesos.hpp 9157ac079808d2686592e54ea26a26e6a0825ed3 Diff: https://reviews.apache.org/r/35947/diff/ Testing --- (1) Added `HierarchicalAllocatorTest.updateAvailableSuccess` and `HierarchicalAllocatorTest.updateAvailableFail` (2) `make check` Thanks, Michael Park
Re: Review Request 35947: Added a new API call 'updateAvailable' to the allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35947/#review90930 --- A high-level concern I would like to share with you guys, though it isn't directly related to this particular patch. I have a feeling that we don't really care about keeping the Allocator interface neat, brief, and concise, but rather abuse it and add random methods to it in order to satisfy our current needs for the built-in allocator. With several production-grade allocators on the market we won't be able to do this anymore. Shall we be more cautious about that? Maybe it is a good time to schedule allocator refactoring? - Alexander Rukletsov On June 26, 2015, 10:55 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35947/ --- (Updated June 26, 2015, 10:55 p.m.) Review request for mesos, Alexander Rukletsov, Benjamin Hindman, Ben Mahler, and Jie Yu. Repository: mesos Description --- Needed to implement the master HTTP endpoints: `/reserve`, `/unreserve`, `/create` and `/destroy`. This API is similar to `updateSlave` which is currently very specific to oversubscription. I considered consolidating `updateAvailable` and `updateSlave` but it will require making offers be generated within the allocator to enable this. In specific, `updateAvailable` could fail if there aren't sufficient available resources to update, whereas `updateSlave` avoids failing by keeping the allocator in an over-allocated state. For `updateSlave`, leaving the allocator in an over-allocated state is ok. This is because it does not modify resources therefore `total - allocated` will work out to __empty__. `updateAvailable` cannot leave the allocator in an over-allocated state, because it modifies resources, and therefore `total - allocated` is not guaranteed to yield __empty__. Diffs - include/mesos/master/allocator.hpp 22992c0c77058af4fcd28aa8e4a1191693a16f44 src/master/allocator/mesos/allocator.hpp 72470ec7f56f84a9a9815c09adb88def90ef672f src/master/allocator/mesos/hierarchical.hpp 3264d145d52b48852878abf7ab9be29ab98208cc src/tests/hierarchical_allocator_tests.cpp 3258840135290cd008ca09235d18b7f093dafd2e src/tests/mesos.hpp 9157ac079808d2686592e54ea26a26e6a0825ed3 Diff: https://reviews.apache.org/r/35947/diff/ Testing --- (1) Added `HierarchicalAllocatorTest.updateAvailableSuccess` and `HierarchicalAllocatorTest.updateAvailableFail` (2) `make check` Thanks, Michael Park
Re: Review Request 35947: Added a new API call 'updateAvailable' to the allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35947/#review90936 --- include/mesos/master/allocator.hpp (lines 133 - 135) https://reviews.apache.org/r/35947/#comment144098 And we introduce a libprocess dependency into `Allocator` interface. I think it's a prominent step, right now an allocator implementation is not even required to be asynchronous. I don't want to say it's a bad change, I want us to think a bit more about the consequences of such step. Maybe it is worth discussing on the dev list. - Alexander Rukletsov On June 26, 2015, 10:55 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35947/ --- (Updated June 26, 2015, 10:55 p.m.) Review request for mesos, Alexander Rukletsov, Benjamin Hindman, Ben Mahler, and Jie Yu. Repository: mesos Description --- Needed to implement the master HTTP endpoints: `/reserve`, `/unreserve`, `/create` and `/destroy`. This API is similar to `updateSlave` which is currently very specific to oversubscription. I considered consolidating `updateAvailable` and `updateSlave` but it will require making offers be generated within the allocator to enable this. In specific, `updateAvailable` could fail if there aren't sufficient available resources to update, whereas `updateSlave` avoids failing by keeping the allocator in an over-allocated state. For `updateSlave`, leaving the allocator in an over-allocated state is ok. This is because it does not modify resources therefore `total - allocated` will work out to __empty__. `updateAvailable` cannot leave the allocator in an over-allocated state, because it modifies resources, and therefore `total - allocated` is not guaranteed to yield __empty__. Diffs - include/mesos/master/allocator.hpp 22992c0c77058af4fcd28aa8e4a1191693a16f44 src/master/allocator/mesos/allocator.hpp 72470ec7f56f84a9a9815c09adb88def90ef672f src/master/allocator/mesos/hierarchical.hpp 3264d145d52b48852878abf7ab9be29ab98208cc src/tests/hierarchical_allocator_tests.cpp 3258840135290cd008ca09235d18b7f093dafd2e src/tests/mesos.hpp 9157ac079808d2686592e54ea26a26e6a0825ed3 Diff: https://reviews.apache.org/r/35947/diff/ Testing --- (1) Added `HierarchicalAllocatorTest.updateAvailableSuccess` and `HierarchicalAllocatorTest.updateAvailableFail` (2) `make check` Thanks, Michael Park
Re: Review Request 35947: Added a new API call 'updateAvailable' to the allocator.
On July 8, 2015, 5:37 p.m., Alexander Rukletsov wrote: A high-level concern I would like to share with you guys, though it isn't directly related to this particular patch. I have a feeling that we don't really care about keeping the Allocator interface neat, brief, and concise, but rather abuse it and add random methods to it in order to satisfy our current needs for the built-in allocator. With several production-grade allocators on the market we won't be able to do this anymore. Shall we be more cautious about that? Maybe it is a good time to schedule allocator refactoring? Jie Yu wrote: Alex, I totally agree with you that the current allocator interface needs a major refactor. Given the recent Quota design, I feel like we should do the refactor asap, otherwise the complexity will soon become unmanagable. I think the key is to define a clear boundary between master and allocator. Currently, the boundary is not clear. For instance, the offer management is done by the master but we have some interfaces like reviveOffers in the allocator. I would suggest that we open a ticket (or a doc) to list all the issues in the current allocator interfaces, and then we can think about solutions. Does that sound good to you? Jie, I'm so glad you share all my unsaid concerns (upcoming quota support, offers management, responsibility separation between the master and the allocator)! I totally agree with your proposal; one thing I would like to add is that it maybe makes sense to raise this concern during the next community sync. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35947/#review90930 --- On June 26, 2015, 10:55 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35947/ --- (Updated June 26, 2015, 10:55 p.m.) Review request for mesos, Alexander Rukletsov, Benjamin Hindman, Ben Mahler, and Jie Yu. Repository: mesos Description --- Needed to implement the master HTTP endpoints: `/reserve`, `/unreserve`, `/create` and `/destroy`. This API is similar to `updateSlave` which is currently very specific to oversubscription. I considered consolidating `updateAvailable` and `updateSlave` but it will require making offers be generated within the allocator to enable this. In specific, `updateAvailable` could fail if there aren't sufficient available resources to update, whereas `updateSlave` avoids failing by keeping the allocator in an over-allocated state. For `updateSlave`, leaving the allocator in an over-allocated state is ok. This is because it does not modify resources therefore `total - allocated` will work out to __empty__. `updateAvailable` cannot leave the allocator in an over-allocated state, because it modifies resources, and therefore `total - allocated` is not guaranteed to yield __empty__. Diffs - include/mesos/master/allocator.hpp 22992c0c77058af4fcd28aa8e4a1191693a16f44 src/master/allocator/mesos/allocator.hpp 72470ec7f56f84a9a9815c09adb88def90ef672f src/master/allocator/mesos/hierarchical.hpp 3264d145d52b48852878abf7ab9be29ab98208cc src/tests/hierarchical_allocator_tests.cpp 3258840135290cd008ca09235d18b7f093dafd2e src/tests/mesos.hpp 9157ac079808d2686592e54ea26a26e6a0825ed3 Diff: https://reviews.apache.org/r/35947/diff/ Testing --- (1) Added `HierarchicalAllocatorTest.updateAvailableSuccess` and `HierarchicalAllocatorTest.updateAvailableFail` (2) `make check` Thanks, Michael Park
Re: Review Request 35947: Added a new API call 'updateAvailable' to the allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35947/#review90839 --- Ship it! src/master/allocator/mesos/hierarchical.hpp (lines 734 - 737) https://reviews.apache.org/r/35947/#comment144004 Could you please add a NOTE here explaining why this is possible? (e.g., it's possible because an allocation can happen after updateAvailable is enqueued but before it's actually being processed). - Jie Yu On June 26, 2015, 10:55 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35947/ --- (Updated June 26, 2015, 10:55 p.m.) Review request for mesos, Alexander Rukletsov, Benjamin Hindman, Ben Mahler, and Jie Yu. Repository: mesos Description --- Needed to implement the master HTTP endpoints: `/reserve`, `/unreserve`, `/create` and `/destroy`. This API is similar to `updateSlave` which is currently very specific to oversubscription. I considered consolidating `updateAvailable` and `updateSlave` but it will require making offers be generated within the allocator to enable this. In specific, `updateAvailable` could fail if there aren't sufficient available resources to update, whereas `updateSlave` avoids failing by keeping the allocator in an over-allocated state. For `updateSlave`, leaving the allocator in an over-allocated state is ok. This is because it does not modify resources therefore `total - allocated` will work out to __empty__. `updateAvailable` cannot leave the allocator in an over-allocated state, because it modifies resources, and therefore `total - allocated` is not guaranteed to yield __empty__. Diffs - include/mesos/master/allocator.hpp 22992c0c77058af4fcd28aa8e4a1191693a16f44 src/master/allocator/mesos/allocator.hpp 72470ec7f56f84a9a9815c09adb88def90ef672f src/master/allocator/mesos/hierarchical.hpp 3264d145d52b48852878abf7ab9be29ab98208cc src/tests/hierarchical_allocator_tests.cpp 3258840135290cd008ca09235d18b7f093dafd2e src/tests/mesos.hpp 9157ac079808d2686592e54ea26a26e6a0825ed3 Diff: https://reviews.apache.org/r/35947/diff/ Testing --- (1) Added `HierarchicalAllocatorTest.updateAvailableSuccess` and `HierarchicalAllocatorTest.updateAvailableFail` (2) `make check` Thanks, Michael Park
Review Request 35947: Added a new API call 'updateAvailable' to the allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35947/ --- Review request for mesos, Alexander Rukletsov, Benjamin Hindman, Ben Mahler, and Jie Yu. Repository: mesos Description --- Needed to implement the master HTTP endpoints: `/reserve`, `/unreserve`, `/create` and `/destroy`. This API is similar to `updateSlave` which is currently very specific to oversubscription. I considered consolidating `updateAvailable` and `updateSlave` but it will require making offers be generated within the allocator to enable this. In specific, `updateAvailable` could fail if there aren't sufficient available resources to update, whereas `updateSlave` avoids failing by keeping the allocator in an over-allocated state. For `updateSlave`, leaving the allocator in an over-allocated state is ok. This is because it does not modify resources therefore `total - allocated` will work out to __empty__. `updateAvailable` cannot leave the allocator in an over-allocated state, because it modifies resources, and therefore `total - allocated` is not guaranteed to yield __empty__. Diffs - include/mesos/master/allocator.hpp 22992c0c77058af4fcd28aa8e4a1191693a16f44 src/master/allocator/mesos/allocator.hpp 72470ec7f56f84a9a9815c09adb88def90ef672f src/master/allocator/mesos/hierarchical.hpp 3264d145d52b48852878abf7ab9be29ab98208cc src/tests/hierarchical_allocator_tests.cpp 3258840135290cd008ca09235d18b7f093dafd2e src/tests/mesos.hpp 9157ac079808d2686592e54ea26a26e6a0825ed3 Diff: https://reviews.apache.org/r/35947/diff/ Testing --- (1) Added `HierarchicalAllocatorTest.updateAvailableSuccess` and `HierarchicalAllocatorTest.updateAvailableFail` (2) `make check` Thanks, Michael Park
