You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Bannier <be...@mesosphere.io> on 2017/07/04 14:40:26 UTC
Review Request 60638: Adjusted allocator interfaces to allow passing
new agent total.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60638/
-----------------------------------------------------------
Review request for mesos, Benjamin Mahler, Jie Yu, and Jan Schlicht.
Bugs: MESOS-7755
https://issues.apache.org/jira/browse/MESOS-7755
Repository: mesos
Description
-------
This change adjust interfaces, implementations and users of the
'updateSlave' method. This allows to pass an additional argument
denoting an updated 'total' amount of resource on the agents.
In a subsequent commit we will update the 'HierarchicalAllocator' to
interpret this value.
Diffs
-----
include/mesos/allocator/allocator.hpp bec9e0b8ef8d51ec22f9e8af69407ed5ecaa3e8f
src/master/allocator/mesos/allocator.hpp 2e780c92d5c5132abff32f1ce051c3bab2947f37
src/master/allocator/mesos/hierarchical.hpp 5c58cf401de9205e54300e0ce8433995cdf5cb7a
src/master/allocator/mesos/hierarchical.cpp eb01d8e6b1108866ebc049f9f4a46157823a3541
src/master/master.cpp 95e9691b3e3ef101e3e7ddc7a498d5c5bf2276e3
src/tests/allocator.hpp a990788d5218bbcac499613783750ade022811a7
src/tests/hierarchical_allocator_tests.cpp 2a312a9af4bae679a0a4e7bf45a3c013513c5da2
Diff: https://reviews.apache.org/r/60638/diff/1/
Testing
-------
Tested with https://reviews.apache.org/r/60639/.
Thanks,
Benjamin Bannier
Re: Review Request 60638: Changed semantics of allocator 'updateSlave'
method.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60638/
-----------------------------------------------------------
(Updated July 8, 2017, 2:31 a.m.)
Review request for mesos, Benjamin Mahler, Jie Yu, and Jan Schlicht.
Changes
-------
Fixed rebase mistake.
Bugs: MESOS-7755
https://issues.apache.org/jira/browse/MESOS-7755
Repository: mesos
Description
-------
We change the semantics of the 'updateSlave' method present in the
allocator interface. While previously the passed optional resource
argument was interpreted as the amount of (new) oversubscribed
resources, it now represents the new amount of total resources on the
given agent.
We addtionally add an optimization of
'HierarchicalAllocatorProcess::updateSlaveTotal' for cases where the
passed total is identical to the current total. This operation is a
no-op now and we prevent updating the sorters.
Diffs (updated)
-----
include/mesos/allocator/allocator.hpp bec9e0b8ef8d51ec22f9e8af69407ed5ecaa3e8f
src/master/allocator/mesos/allocator.hpp 2e780c92d5c5132abff32f1ce051c3bab2947f37
src/master/allocator/mesos/hierarchical.hpp 5c58cf401de9205e54300e0ce8433995cdf5cb7a
src/master/allocator/mesos/hierarchical.cpp eb01d8e6b1108866ebc049f9f4a46157823a3541
src/master/master.cpp 56b170ed70722eba30d98f6e648b6a31580d6b56
src/tests/hierarchical_allocator_tests.cpp 2a312a9af4bae679a0a4e7bf45a3c013513c5da2
Diff: https://reviews.apache.org/r/60638/diff/5/
Changes: https://reviews.apache.org/r/60638/diff/4-5/
Testing
-------
Tested with https://reviews.apache.org/r/60639/.
Thanks,
Benjamin Bannier
Re: Review Request 60638: Changed semantics of allocator 'updateSlave'
method.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60638/
-----------------------------------------------------------
(Updated July 7, 2017, 9:38 p.m.)
Review request for mesos, Benjamin Mahler, Jie Yu, and Jan Schlicht.
Changes
-------
Fixed handling of `optional` proto field.
Previously we implicitly converted not set `type` fields to `UNKNOWN`.
Since the field is `optional` we can actually distinguish between an
unset and an `UNKNOWN` field. We now treat messages with unset `type`
as `OVERSUBSCRIBED`, but reject `type` `UNKNOWN` since it is an
indication of an incompatible caller.
Bugs: MESOS-7755
https://issues.apache.org/jira/browse/MESOS-7755
Repository: mesos
Description
-------
We change the semantics of the 'updateSlave' method present in the
allocator interface. While previously the passed optional resource
argument was interpreted as the amount of (new) oversubscribed
resources, it now represents the new amount of total resources on the
given agent.
We addtionally add an optimization of
'HierarchicalAllocatorProcess::updateSlaveTotal' for cases where the
passed total is identical to the current total. This operation is a
no-op now and we prevent updating the sorters.
Diffs (updated)
-----
include/mesos/allocator/allocator.hpp bec9e0b8ef8d51ec22f9e8af69407ed5ecaa3e8f
src/master/allocator/mesos/allocator.hpp 2e780c92d5c5132abff32f1ce051c3bab2947f37
src/master/allocator/mesos/hierarchical.hpp 5c58cf401de9205e54300e0ce8433995cdf5cb7a
src/master/allocator/mesos/hierarchical.cpp eb01d8e6b1108866ebc049f9f4a46157823a3541
src/master/master.hpp 95c2d0fab32d6b60f29a86037607ff009bd78717
src/master/master.cpp 56b170ed70722eba30d98f6e648b6a31580d6b56
src/tests/hierarchical_allocator_tests.cpp 2a312a9af4bae679a0a4e7bf45a3c013513c5da2
Diff: https://reviews.apache.org/r/60638/diff/4/
Changes: https://reviews.apache.org/r/60638/diff/3-4/
Testing
-------
Tested with https://reviews.apache.org/r/60639/.
Thanks,
Benjamin Bannier
Re: Review Request 60638: Changed semantics of allocator 'updateSlave'
method.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60638/
-----------------------------------------------------------
(Updated July 7, 2017, 10:54 a.m.)
Review request for mesos, Benjamin Mahler, Jie Yu, and Jan Schlicht.
Changes
-------
Added optimization for cases where new total == old total.
Bugs: MESOS-7755
https://issues.apache.org/jira/browse/MESOS-7755
Repository: mesos
Description (updated)
-------
We change the semantics of the 'updateSlave' method present in the
allocator interface. While previously the passed optional resource
argument was interpreted as the amount of (new) oversubscribed
resources, it now represents the new amount of total resources on the
given agent.
We addtionally add an optimization of
'HierarchicalAllocatorProcess::updateSlaveTotal' for cases where the
passed total is identical to the current total. This operation is a
no-op now and we prevent updating the sorters.
Diffs (updated)
-----
include/mesos/allocator/allocator.hpp bec9e0b8ef8d51ec22f9e8af69407ed5ecaa3e8f
src/master/allocator/mesos/allocator.hpp 2e780c92d5c5132abff32f1ce051c3bab2947f37
src/master/allocator/mesos/hierarchical.hpp 5c58cf401de9205e54300e0ce8433995cdf5cb7a
src/master/allocator/mesos/hierarchical.cpp eb01d8e6b1108866ebc049f9f4a46157823a3541
src/master/master.cpp 56b170ed70722eba30d98f6e648b6a31580d6b56
src/tests/hierarchical_allocator_tests.cpp 2a312a9af4bae679a0a4e7bf45a3c013513c5da2
Diff: https://reviews.apache.org/r/60638/diff/3/
Changes: https://reviews.apache.org/r/60638/diff/2-3/
Testing
-------
Tested with https://reviews.apache.org/r/60639/.
Thanks,
Benjamin Bannier
Re: Review Request 60638: Changed semantics of allocator 'updateSlave'
method.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On July 7, 2017, 5:54 a.m., Jie Yu wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Line 652 (original), 652 (patched)
> > <https://reviews.apache.org/r/60638/diff/2/?file=1770519#file1770519line652>
> >
> > Let's update `updateSlaveTotal` so that if the old total is the same as the new total, we don't need to update the sorter. That means updateSlaveTotal should probably return a bool to indicate if there is any change.
> >
> > This is a great cleanup! Much cleaner now!
Cleaned up.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60638/#review179845
-----------------------------------------------------------
On July 7, 2017, 10:54 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60638/
> -----------------------------------------------------------
>
> (Updated July 7, 2017, 10:54 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jie Yu, and Jan Schlicht.
>
>
> Bugs: MESOS-7755
> https://issues.apache.org/jira/browse/MESOS-7755
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We change the semantics of the 'updateSlave' method present in the
> allocator interface. While previously the passed optional resource
> argument was interpreted as the amount of (new) oversubscribed
> resources, it now represents the new amount of total resources on the
> given agent.
>
> We addtionally add an optimization of
> 'HierarchicalAllocatorProcess::updateSlaveTotal' for cases where the
> passed total is identical to the current total. This operation is a
> no-op now and we prevent updating the sorters.
>
>
> Diffs
> -----
>
> include/mesos/allocator/allocator.hpp bec9e0b8ef8d51ec22f9e8af69407ed5ecaa3e8f
> src/master/allocator/mesos/allocator.hpp 2e780c92d5c5132abff32f1ce051c3bab2947f37
> src/master/allocator/mesos/hierarchical.hpp 5c58cf401de9205e54300e0ce8433995cdf5cb7a
> src/master/allocator/mesos/hierarchical.cpp eb01d8e6b1108866ebc049f9f4a46157823a3541
> src/master/master.cpp 56b170ed70722eba30d98f6e648b6a31580d6b56
> src/tests/hierarchical_allocator_tests.cpp 2a312a9af4bae679a0a4e7bf45a3c013513c5da2
>
>
> Diff: https://reviews.apache.org/r/60638/diff/3/
>
>
> Testing
> -------
>
> Tested with https://reviews.apache.org/r/60639/.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 60638: Changed semantics of allocator 'updateSlave'
method.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60638/#review179845
-----------------------------------------------------------
Fix it, then Ship it!
src/master/allocator/mesos/hierarchical.cpp
Line 652 (original), 652 (patched)
<https://reviews.apache.org/r/60638/#comment254742>
Let's update `updateSlaveTotal` so that if the old total is the same as the new total, we don't need to update the sorter. That means updateSlaveTotal should probably return a bool to indicate if there is any change.
This is a great cleanup! Much cleaner now!
- Jie Yu
On July 6, 2017, 4:35 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60638/
> -----------------------------------------------------------
>
> (Updated July 6, 2017, 4:35 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jie Yu, and Jan Schlicht.
>
>
> Bugs: MESOS-7755
> https://issues.apache.org/jira/browse/MESOS-7755
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We change the semantics of the 'updateSlave' method present in the
> allocator interface. While previously the passed optional resource
> argument was interpreted as the amount of (new) oversubscribed
> resources, it now represents the new amount of total resources on the
> given agent.
>
>
> Diffs
> -----
>
> include/mesos/allocator/allocator.hpp bec9e0b8ef8d51ec22f9e8af69407ed5ecaa3e8f
> src/master/allocator/mesos/allocator.hpp 2e780c92d5c5132abff32f1ce051c3bab2947f37
> src/master/allocator/mesos/hierarchical.hpp 5c58cf401de9205e54300e0ce8433995cdf5cb7a
> src/master/allocator/mesos/hierarchical.cpp eb01d8e6b1108866ebc049f9f4a46157823a3541
> src/master/master.cpp 95e9691b3e3ef101e3e7ddc7a498d5c5bf2276e3
> src/tests/hierarchical_allocator_tests.cpp 2a312a9af4bae679a0a4e7bf45a3c013513c5da2
>
>
> Diff: https://reviews.apache.org/r/60638/diff/2/
>
>
> Testing
> -------
>
> Tested with https://reviews.apache.org/r/60639/.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 60638: Changed semantics of allocator 'updateSlave'
method.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60638/
-----------------------------------------------------------
(Updated July 6, 2017, 6:35 p.m.)
Review request for mesos, Benjamin Mahler, Jie Yu, and Jan Schlicht.
Changes
-------
Changed semantics of `Allocator::updateSlave` instead of adding a semantically very close `total` resources in addition to the existing `oversubscribed`.
Summary (updated)
-----------------
Changed semantics of allocator 'updateSlave' method.
Bugs: MESOS-7755
https://issues.apache.org/jira/browse/MESOS-7755
Repository: mesos
Description (updated)
-------
We change the semantics of the 'updateSlave' method present in the
allocator interface. While previously the passed optional resource
argument was interpreted as the amount of (new) oversubscribed
resources, it now represents the new amount of total resources on the
given agent.
Diffs (updated)
-----
include/mesos/allocator/allocator.hpp bec9e0b8ef8d51ec22f9e8af69407ed5ecaa3e8f
src/master/allocator/mesos/allocator.hpp 2e780c92d5c5132abff32f1ce051c3bab2947f37
src/master/allocator/mesos/hierarchical.hpp 5c58cf401de9205e54300e0ce8433995cdf5cb7a
src/master/allocator/mesos/hierarchical.cpp eb01d8e6b1108866ebc049f9f4a46157823a3541
src/master/master.cpp 95e9691b3e3ef101e3e7ddc7a498d5c5bf2276e3
src/tests/hierarchical_allocator_tests.cpp 2a312a9af4bae679a0a4e7bf45a3c013513c5da2
Diff: https://reviews.apache.org/r/60638/diff/2/
Changes: https://reviews.apache.org/r/60638/diff/1-2/
Testing
-------
Tested with https://reviews.apache.org/r/60639/.
Thanks,
Benjamin Bannier
Re: Review Request 60638: Changed semantics of allocator 'updateSlave'
method.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On July 5, 2017, 7:34 p.m., Jie Yu wrote:
> > include/mesos/allocator/allocator.hpp
> > Line 219 (original), 220-221 (patched)
> > <https://reviews.apache.org/r/60638/diff/1/?file=1769166#file1769166line220>
> >
> > It's quite confusing to the readers when there are two fields there. Does 'total' include 'oversubscribed' resources?
> >
> > I am wondering if we can combine these two fields in the allocator. Updating 'oversubscribed' is a special case of updating 'total'?
I changed this to only pass a total. This requires changes to the existing `UpdateSlaveMessage` before adding this behavior, these changes are done in https://reviews.apache.org/r/60641/.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60638/#review179659
-----------------------------------------------------------
On July 6, 2017, 6:35 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60638/
> -----------------------------------------------------------
>
> (Updated July 6, 2017, 6:35 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jie Yu, and Jan Schlicht.
>
>
> Bugs: MESOS-7755
> https://issues.apache.org/jira/browse/MESOS-7755
>
>
> Repository: mesos
>
>
> Description
> -------
>
> We change the semantics of the 'updateSlave' method present in the
> allocator interface. While previously the passed optional resource
> argument was interpreted as the amount of (new) oversubscribed
> resources, it now represents the new amount of total resources on the
> given agent.
>
>
> Diffs
> -----
>
> include/mesos/allocator/allocator.hpp bec9e0b8ef8d51ec22f9e8af69407ed5ecaa3e8f
> src/master/allocator/mesos/allocator.hpp 2e780c92d5c5132abff32f1ce051c3bab2947f37
> src/master/allocator/mesos/hierarchical.hpp 5c58cf401de9205e54300e0ce8433995cdf5cb7a
> src/master/allocator/mesos/hierarchical.cpp eb01d8e6b1108866ebc049f9f4a46157823a3541
> src/master/master.cpp 95e9691b3e3ef101e3e7ddc7a498d5c5bf2276e3
> src/tests/hierarchical_allocator_tests.cpp 2a312a9af4bae679a0a4e7bf45a3c013513c5da2
>
>
> Diff: https://reviews.apache.org/r/60638/diff/2/
>
>
> Testing
> -------
>
> Tested with https://reviews.apache.org/r/60639/.
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 60638: Adjusted allocator interfaces to allow
passing new agent total.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60638/#review179659
-----------------------------------------------------------
include/mesos/allocator/allocator.hpp
Line 219 (original), 220-221 (patched)
<https://reviews.apache.org/r/60638/#comment254468>
It's quite confusing to the readers when there are two fields there. Does 'total' include 'oversubscribed' resources?
I am wondering if we can combine these two fields in the allocator. Updating 'oversubscribed' is a special case of updating 'total'?
- Jie Yu
On July 4, 2017, 2:40 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60638/
> -----------------------------------------------------------
>
> (Updated July 4, 2017, 2:40 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jie Yu, and Jan Schlicht.
>
>
> Bugs: MESOS-7755
> https://issues.apache.org/jira/browse/MESOS-7755
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This change adjust interfaces, implementations and users of the
> 'updateSlave' method. This allows to pass an additional argument
> denoting an updated 'total' amount of resource on the agents.
>
> In a subsequent commit we will update the 'HierarchicalAllocator' to
> interpret this value.
>
>
> Diffs
> -----
>
> include/mesos/allocator/allocator.hpp bec9e0b8ef8d51ec22f9e8af69407ed5ecaa3e8f
> src/master/allocator/mesos/allocator.hpp 2e780c92d5c5132abff32f1ce051c3bab2947f37
> src/master/allocator/mesos/hierarchical.hpp 5c58cf401de9205e54300e0ce8433995cdf5cb7a
> src/master/allocator/mesos/hierarchical.cpp eb01d8e6b1108866ebc049f9f4a46157823a3541
> src/master/master.cpp 95e9691b3e3ef101e3e7ddc7a498d5c5bf2276e3
> src/tests/allocator.hpp a990788d5218bbcac499613783750ade022811a7
> src/tests/hierarchical_allocator_tests.cpp 2a312a9af4bae679a0a4e7bf45a3c013513c5da2
>
>
> Diff: https://reviews.apache.org/r/60638/diff/1/
>
>
> Testing
> -------
>
> Tested with https://reviews.apache.org/r/60639/.
>
>
> Thanks,
>
> Benjamin Bannier
>
>