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/06 16:36:06 UTC
Re: Review Request 60641: Allowed to pass total resources in
'UpdateSlaveMessage'.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60641/
-----------------------------------------------------------
(Updated July 6, 2017, 6:36 p.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Rebased.
Summary (updated)
-----------------
Allowed to pass total resources in 'UpdateSlaveMessage'.
Bugs: MESOS-7757
https://issues.apache.org/jira/browse/MESOS-7757
Repository: mesos
Description (updated)
-------
This commit both extends the existing 'UpdateSlaveMessage' proto
message, and adjusts its handling on the agent and master side.
To distinguish updates to 'oversubscribed_resources' from updates to
'total_resources' the message now contains a 'type' field to allow
disambiguation among an empty list of resources and an unset list of
resources. For backwards-compatibility we assume whenever the type
field was not set that caller intended to use the
'oversubscribed_resources' field as opposed to the 'total_resources'
field.
Currently, passing 'total_resources' is handled neither in the master
nor the default allocator; we will implement this in a subsequent
patch.
Diffs (updated)
-----
src/master/master.hpp 9dd6a530c373516dc81bfd51ee6e95f25588148f
src/master/master.cpp 95e9691b3e3ef101e3e7ddc7a498d5c5bf2276e3
src/messages/messages.proto 67cee3146a45f8f720b4e60739cf85085d18259b
src/slave/slave.cpp 0e24b8cb8d1020af515e3d1862e121e1daf82ce9
src/tests/oversubscription_tests.cpp 2266510091b49c61310a363ab6644708655787dc
Diff: https://reviews.apache.org/r/60641/diff/2/
Changes: https://reviews.apache.org/r/60641/diff/1-2/
Testing
-------
Thanks,
Benjamin Bannier
Re: Review Request 60641: Allowed to pass total resources in
'UpdateSlaveMessage'.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60641/
-----------------------------------------------------------
(Updated July 8, 2017, 2:31 a.m.)
Review request for mesos, 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-7757
https://issues.apache.org/jira/browse/MESOS-7757
Repository: mesos
Description
-------
This commit both extends the existing 'UpdateSlaveMessage' proto
message, and adjusts its handling on the agent and master side.
To distinguish updates to 'oversubscribed_resources' from updates to
'total_resources' the message now contains a 'type' field to allow
disambiguation among an empty list of resources and an unset list of
resources. For backwards-compatibility we assume whenever the type
field was not set that caller intended to use the
'oversubscribed_resources' field as opposed to the 'total_resources'
field.
Currently, passing 'total_resources' is handled neither in the master
nor the default allocator; we will implement this in a subsequent
patch.
Diffs (updated)
-----
src/master/master.hpp 95c2d0fab32d6b60f29a86037607ff009bd78717
src/master/master.cpp 56b170ed70722eba30d98f6e648b6a31580d6b56
src/messages/messages.proto 67cee3146a45f8f720b4e60739cf85085d18259b
src/slave/slave.cpp 52f673633fa0041259ae40f88d4c0281a1e053b0
src/tests/oversubscription_tests.cpp 2266510091b49c61310a363ab6644708655787dc
Diff: https://reviews.apache.org/r/60641/diff/4/
Changes: https://reviews.apache.org/r/60641/diff/3-4/
Testing
-------
Thanks,
Benjamin Bannier
Re: Review Request 60641: Allowed to pass total resources in
'UpdateSlaveMessage'.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60641/#review179894
-----------------------------------------------------------
Fix it, then Ship it!
src/master/master.cpp
Line 900 (original), 901 (patched)
<https://reviews.apache.org/r/60641/#comment254801>
Given that 'type' here might be optional. It's weird that we use this overload of 'install'.
Let's just use the install variant that takes the protobuf message itself.
```
install<UpdateSlaveMessage>(&Master::updateSlave);
void Master::updateSlave(const UpdateSlaveMessage& message);
```
- Jie Yu
On July 7, 2017, 8:54 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60641/
> -----------------------------------------------------------
>
> (Updated July 7, 2017, 8:54 a.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Bugs: MESOS-7757
> https://issues.apache.org/jira/browse/MESOS-7757
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This commit both extends the existing 'UpdateSlaveMessage' proto
> message, and adjusts its handling on the agent and master side.
>
> To distinguish updates to 'oversubscribed_resources' from updates to
> 'total_resources' the message now contains a 'type' field to allow
> disambiguation among an empty list of resources and an unset list of
> resources. For backwards-compatibility we assume whenever the type
> field was not set that caller intended to use the
> 'oversubscribed_resources' field as opposed to the 'total_resources'
> field.
>
> Currently, passing 'total_resources' is handled neither in the master
> nor the default allocator; we will implement this in a subsequent
> patch.
>
>
> Diffs
> -----
>
> src/master/master.hpp 95c2d0fab32d6b60f29a86037607ff009bd78717
> src/master/master.cpp 56b170ed70722eba30d98f6e648b6a31580d6b56
> src/messages/messages.proto 67cee3146a45f8f720b4e60739cf85085d18259b
> src/slave/slave.cpp 52f673633fa0041259ae40f88d4c0281a1e053b0
> src/tests/oversubscription_tests.cpp 2266510091b49c61310a363ab6644708655787dc
>
>
> Diff: https://reviews.apache.org/r/60641/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 60641: Allowed to pass total resources in
'UpdateSlaveMessage'.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60641/
-----------------------------------------------------------
(Updated July 7, 2017, 10:54 a.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Made new `required` field `optional`.
Bugs: MESOS-7757
https://issues.apache.org/jira/browse/MESOS-7757
Repository: mesos
Description
-------
This commit both extends the existing 'UpdateSlaveMessage' proto
message, and adjusts its handling on the agent and master side.
To distinguish updates to 'oversubscribed_resources' from updates to
'total_resources' the message now contains a 'type' field to allow
disambiguation among an empty list of resources and an unset list of
resources. For backwards-compatibility we assume whenever the type
field was not set that caller intended to use the
'oversubscribed_resources' field as opposed to the 'total_resources'
field.
Currently, passing 'total_resources' is handled neither in the master
nor the default allocator; we will implement this in a subsequent
patch.
Diffs (updated)
-----
src/master/master.hpp 95c2d0fab32d6b60f29a86037607ff009bd78717
src/master/master.cpp 56b170ed70722eba30d98f6e648b6a31580d6b56
src/messages/messages.proto 67cee3146a45f8f720b4e60739cf85085d18259b
src/slave/slave.cpp 52f673633fa0041259ae40f88d4c0281a1e053b0
src/tests/oversubscription_tests.cpp 2266510091b49c61310a363ab6644708655787dc
Diff: https://reviews.apache.org/r/60641/diff/3/
Changes: https://reviews.apache.org/r/60641/diff/2-3/
Testing
-------
Thanks,
Benjamin Bannier
Re: Review Request 60641: Allowed to pass total resources in
'UpdateSlaveMessage'.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On July 7, 2017, 5:41 a.m., Jie Yu wrote:
> > src/messages/messages.proto
> > Lines 628 (patched)
> > <https://reviews.apache.org/r/60641/diff/2/?file=1770513#file1770513line628>
> >
> > s/required/optional/
> >
> > See why enum should be optional in MESOS-4997
I made this field `optional` now.
> On July 7, 2017, 5:41 a.m., Jie Yu wrote:
> > src/slave/slave.cpp
> > Lines 1233 (patched)
> > <https://reviews.apache.org/r/60641/diff/2/?file=1770514#file1770514line1233>
> >
> > Then, you don't have to change these if type is optional.
I would prefer to still explicitly set this field as that is the now expected usage pattern. We should only automigrate `UNKNOWN` -> `OVERSUBSCRIBED` for backwards-compatibility reasons external users, not to save tying.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60641/#review179844
-----------------------------------------------------------
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/60641/
> -----------------------------------------------------------
>
> (Updated July 7, 2017, 10:54 a.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Bugs: MESOS-7757
> https://issues.apache.org/jira/browse/MESOS-7757
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This commit both extends the existing 'UpdateSlaveMessage' proto
> message, and adjusts its handling on the agent and master side.
>
> To distinguish updates to 'oversubscribed_resources' from updates to
> 'total_resources' the message now contains a 'type' field to allow
> disambiguation among an empty list of resources and an unset list of
> resources. For backwards-compatibility we assume whenever the type
> field was not set that caller intended to use the
> 'oversubscribed_resources' field as opposed to the 'total_resources'
> field.
>
> Currently, passing 'total_resources' is handled neither in the master
> nor the default allocator; we will implement this in a subsequent
> patch.
>
>
> Diffs
> -----
>
> src/master/master.hpp 95c2d0fab32d6b60f29a86037607ff009bd78717
> src/master/master.cpp 56b170ed70722eba30d98f6e648b6a31580d6b56
> src/messages/messages.proto 67cee3146a45f8f720b4e60739cf85085d18259b
> src/slave/slave.cpp 52f673633fa0041259ae40f88d4c0281a1e053b0
> src/tests/oversubscription_tests.cpp 2266510091b49c61310a363ab6644708655787dc
>
>
> Diff: https://reviews.apache.org/r/60641/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 60641: Allowed to pass total resources in
'UpdateSlaveMessage'.
Posted by Benjamin Bannier <be...@mesosphere.io>.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60641/#review179844
-----------------------------------------------------------
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/60641/
> -----------------------------------------------------------
>
> (Updated July 7, 2017, 10:54 a.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Bugs: MESOS-7757
> https://issues.apache.org/jira/browse/MESOS-7757
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This commit both extends the existing 'UpdateSlaveMessage' proto
> message, and adjusts its handling on the agent and master side.
>
> To distinguish updates to 'oversubscribed_resources' from updates to
> 'total_resources' the message now contains a 'type' field to allow
> disambiguation among an empty list of resources and an unset list of
> resources. For backwards-compatibility we assume whenever the type
> field was not set that caller intended to use the
> 'oversubscribed_resources' field as opposed to the 'total_resources'
> field.
>
> Currently, passing 'total_resources' is handled neither in the master
> nor the default allocator; we will implement this in a subsequent
> patch.
>
>
> Diffs
> -----
>
> src/master/master.hpp 95c2d0fab32d6b60f29a86037607ff009bd78717
> src/master/master.cpp 56b170ed70722eba30d98f6e648b6a31580d6b56
> src/messages/messages.proto 67cee3146a45f8f720b4e60739cf85085d18259b
> src/slave/slave.cpp 52f673633fa0041259ae40f88d4c0281a1e053b0
> src/tests/oversubscription_tests.cpp 2266510091b49c61310a363ab6644708655787dc
>
>
> Diff: https://reviews.apache.org/r/60641/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 60641: Allowed to pass total resources in
'UpdateSlaveMessage'.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60641/#review179844
-----------------------------------------------------------
Fix it, then Ship it!
src/messages/messages.proto
Lines 628 (patched)
<https://reviews.apache.org/r/60641/#comment254740>
s/required/optional/
See why enum should be optional in MESOS-4997
src/slave/slave.cpp
Lines 1233 (patched)
<https://reviews.apache.org/r/60641/#comment254741>
Then, you don't have to change these if type is optional.
- Jie Yu
On July 6, 2017, 4:36 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60641/
> -----------------------------------------------------------
>
> (Updated July 6, 2017, 4:36 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Bugs: MESOS-7757
> https://issues.apache.org/jira/browse/MESOS-7757
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This commit both extends the existing 'UpdateSlaveMessage' proto
> message, and adjusts its handling on the agent and master side.
>
> To distinguish updates to 'oversubscribed_resources' from updates to
> 'total_resources' the message now contains a 'type' field to allow
> disambiguation among an empty list of resources and an unset list of
> resources. For backwards-compatibility we assume whenever the type
> field was not set that caller intended to use the
> 'oversubscribed_resources' field as opposed to the 'total_resources'
> field.
>
> Currently, passing 'total_resources' is handled neither in the master
> nor the default allocator; we will implement this in a subsequent
> patch.
>
>
> Diffs
> -----
>
> src/master/master.hpp 9dd6a530c373516dc81bfd51ee6e95f25588148f
> src/master/master.cpp 95e9691b3e3ef101e3e7ddc7a498d5c5bf2276e3
> src/messages/messages.proto 67cee3146a45f8f720b4e60739cf85085d18259b
> src/slave/slave.cpp 0e24b8cb8d1020af515e3d1862e121e1daf82ce9
> src/tests/oversubscription_tests.cpp 2266510091b49c61310a363ab6644708655787dc
>
>
> Diff: https://reviews.apache.org/r/60641/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benjamin Bannier
>
>