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/12/07 17:49:53 UTC
Review Request 64422: Added explicit resource provider information to
'UpdateSlaveMessage'.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64422/
-----------------------------------------------------------
Review request for mesos, Jie Yu and Jan Schlicht.
Repository: mesos
Description
-------
The added fields will allow us to explicitly surface resource
provider-related information in the master.
Diffs
-----
src/messages/messages.proto 2ab0fe8400a2de2318368d1b408b266ce647339d
Diff: https://reviews.apache.org/r/64422/diff/1/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 64422: Added explicit resource provider
information to 'UpdateSlaveMessage'.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64422/
-----------------------------------------------------------
(Updated Dec. 8, 2017, 12:32 p.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Rebased.
Bugs: MESOS-8312
https://issues.apache.org/jira/browse/MESOS-8312
Repository: mesos
Description
-------
The added fields will allow us to explicitly surface resource
provider-related information in the master.
Diffs (updated)
-----
src/messages/messages.proto f71178438660fa16aad8f290391baba7813fbff0
Diff: https://reviews.apache.org/r/64422/diff/3/
Changes: https://reviews.apache.org/r/64422/diff/2-3/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 64422: Added explicit resource provider
information to 'UpdateSlaveMessage'.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64422/#review193215
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On Dec. 8, 2017, 12:17 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64422/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2017, 12:17 a.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Bugs: MESOS-8312
> https://issues.apache.org/jira/browse/MESOS-8312
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The added fields will allow us to explicitly surface resource
> provider-related information in the master.
>
>
> Diffs
> -----
>
> src/messages/messages.proto 7ab07d7a78210f4a6f418458b1484d9301710dd3
>
>
> Diff: https://reviews.apache.org/r/64422/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 64422: Added explicit resource provider
information to 'UpdateSlaveMessage'.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64422/
-----------------------------------------------------------
(Updated Dec. 8, 2017, 1:17 a.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Addressed comments from Jie.
Bugs: MESOS-8312
https://issues.apache.org/jira/browse/MESOS-8312
Repository: mesos
Description
-------
The added fields will allow us to explicitly surface resource
provider-related information in the master.
Diffs (updated)
-----
src/messages/messages.proto 7ab07d7a78210f4a6f418458b1484d9301710dd3
Diff: https://reviews.apache.org/r/64422/diff/2/
Changes: https://reviews.apache.org/r/64422/diff/1-2/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 64422: Added explicit resource provider
information to 'UpdateSlaveMessage'.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Dec. 7, 2017, 7:47 p.m., Jie Yu wrote:
> > src/messages/messages.proto
> > Line 714 (original), 714 (patched)
> > <https://reviews.apache.org/r/64422/diff/1/?file=1910528#file1910528line714>
> >
> > Should we change this to
> > `optional bytes resource_version_uuid`?
>
> Benjamin Bannier wrote:
> I we pass agent information as a `ResourceProvider` this field can go away.
I changed this field to only contain a single value in https://reviews.apache.org/r/64430/.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64422/#review193148
-----------------------------------------------------------
On Dec. 8, 2017, 1:17 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64422/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2017, 1:17 a.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Bugs: MESOS-8312
> https://issues.apache.org/jira/browse/MESOS-8312
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The added fields will allow us to explicitly surface resource
> provider-related information in the master.
>
>
> Diffs
> -----
>
> src/messages/messages.proto 7ab07d7a78210f4a6f418458b1484d9301710dd3
>
>
> Diff: https://reviews.apache.org/r/64422/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 64422: Added explicit resource provider
information to 'UpdateSlaveMessage'.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Dec. 7, 2017, 7:47 p.m., Jie Yu wrote:
> > src/messages/messages.proto
> > Line 714 (original), 714 (patched)
> > <https://reviews.apache.org/r/64422/diff/1/?file=1910528#file1910528line714>
> >
> > Should we change this to
> > `optional bytes resource_version_uuid`?
I we pass agent information as a `ResourceProvider` this field can go away.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64422/#review193148
-----------------------------------------------------------
On Dec. 7, 2017, 6:49 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64422/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2017, 6:49 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The added fields will allow us to explicitly surface resource
> provider-related information in the master.
>
>
> Diffs
> -----
>
> src/messages/messages.proto 2ab0fe8400a2de2318368d1b408b266ce647339d
>
>
> Diff: https://reviews.apache.org/r/64422/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 64422: Added explicit resource provider
information to 'UpdateSlaveMessage'.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64422/#review193148
-----------------------------------------------------------
src/messages/messages.proto
Line 714 (original), 714 (patched)
<https://reviews.apache.org/r/64422/#comment271651>
Should we change this to
`optional bytes resource_version_uuid`?
- Jie Yu
On Dec. 7, 2017, 5:49 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64422/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2017, 5:49 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The added fields will allow us to explicitly surface resource
> provider-related information in the master.
>
>
> Diffs
> -----
>
> src/messages/messages.proto 2ab0fe8400a2de2318368d1b408b266ce647339d
>
>
> Diff: https://reviews.apache.org/r/64422/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 64422: Added explicit resource provider
information to 'UpdateSlaveMessage'.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Dec. 7, 2017, 7:37 p.m., Jie Yu wrote:
> > src/messages/messages.proto
> > Lines 720-721 (patched)
> > <https://reviews.apache.org/r/64422/diff/1/?file=1910528#file1910528line720>
> >
> > any reason those are optional?
They are required now.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64422/#review193146
-----------------------------------------------------------
On Dec. 8, 2017, 1:17 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64422/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2017, 1:17 a.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Bugs: MESOS-8312
> https://issues.apache.org/jira/browse/MESOS-8312
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The added fields will allow us to explicitly surface resource
> provider-related information in the master.
>
>
> Diffs
> -----
>
> src/messages/messages.proto 7ab07d7a78210f4a6f418458b1484d9301710dd3
>
>
> Diff: https://reviews.apache.org/r/64422/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 64422: Added explicit resource provider
information to 'UpdateSlaveMessage'.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Dec. 7, 2017, 7:37 p.m., Jie Yu wrote:
> > src/messages/messages.proto
> > Lines 693-701 (original), 693-701 (patched)
> > <https://reviews.apache.org/r/64422/diff/1/?file=1910528#file1910528line693>
> >
> > So the idea for these fields are for agent default resources? If yes, can you please add some comments about that?
>
> Jie Yu wrote:
> ah, i saw you removed it in a subsequent patch.
Ideally we would remove the top-level `offer_operations` field and just pass agent information as a resource provider without `ResourceProviderInfo`. I left this in for the time being, but it would be relatively straight-forward for the master to accept either format.
> On Dec. 7, 2017, 7:37 p.m., Jie Yu wrote:
> > src/messages/messages.proto
> > Lines 718 (patched)
> > <https://reviews.apache.org/r/64422/diff/1/?file=1910528#file1910528line718>
> >
> > why optional?
If we ever want to pass agent information in this field we need to either create a `ResourceProviderInfo` for the agent, or leave it `optional`. I went with `optional` since it allows us to defer that decision until we make that change (we could either make an info up, or leave it unset).
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64422/#review193146
-----------------------------------------------------------
On Dec. 7, 2017, 6:49 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64422/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2017, 6:49 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The added fields will allow us to explicitly surface resource
> provider-related information in the master.
>
>
> Diffs
> -----
>
> src/messages/messages.proto 2ab0fe8400a2de2318368d1b408b266ce647339d
>
>
> Diff: https://reviews.apache.org/r/64422/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 64422: Added explicit resource provider
information to 'UpdateSlaveMessage'.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 7, 2017, 6:37 p.m., Jie Yu wrote:
> > src/messages/messages.proto
> > Lines 693-701 (original), 693-701 (patched)
> > <https://reviews.apache.org/r/64422/diff/1/?file=1910528#file1910528line693>
> >
> > So the idea for these fields are for agent default resources? If yes, can you please add some comments about that?
ah, i saw you removed it in a subsequent patch.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64422/#review193146
-----------------------------------------------------------
On Dec. 7, 2017, 5:49 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64422/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2017, 5:49 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The added fields will allow us to explicitly surface resource
> provider-related information in the master.
>
>
> Diffs
> -----
>
> src/messages/messages.proto 2ab0fe8400a2de2318368d1b408b266ce647339d
>
>
> Diff: https://reviews.apache.org/r/64422/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 64422: Added explicit resource provider
information to 'UpdateSlaveMessage'.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 7, 2017, 6:37 p.m., Jie Yu wrote:
> > src/messages/messages.proto
> > Lines 693-701 (original), 693-701 (patched)
> > <https://reviews.apache.org/r/64422/diff/1/?file=1910528#file1910528line693>
> >
> > So the idea for these fields are for agent default resources? If yes, can you please add some comments about that?
>
> Jie Yu wrote:
> ah, i saw you removed it in a subsequent patch.
>
> Benjamin Bannier wrote:
> Ideally we would remove the top-level `offer_operations` field and just pass agent information as a resource provider without `ResourceProviderInfo`. I left this in for the time being, but it would be relatively straight-forward for the master to accept either format.
> Ideally we would remove the top-level offer_operations field and just pass agent information as a resource provider without ResourceProviderInfo.
I won't go that far yet. We probably still keep top level offer_operations to represent operations for the agent default resources.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64422/#review193146
-----------------------------------------------------------
On Dec. 7, 2017, 5:49 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64422/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2017, 5:49 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The added fields will allow us to explicitly surface resource
> provider-related information in the master.
>
>
> Diffs
> -----
>
> src/messages/messages.proto 2ab0fe8400a2de2318368d1b408b266ce647339d
>
>
> Diff: https://reviews.apache.org/r/64422/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 64422: Added explicit resource provider
information to 'UpdateSlaveMessage'.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64422/#review193146
-----------------------------------------------------------
src/messages/messages.proto
Lines 693-701 (original), 693-701 (patched)
<https://reviews.apache.org/r/64422/#comment271647>
So the idea for these fields are for agent default resources? If yes, can you please add some comments about that?
src/messages/messages.proto
Lines 718 (patched)
<https://reviews.apache.org/r/64422/#comment271649>
why optional?
src/messages/messages.proto
Lines 720-721 (patched)
<https://reviews.apache.org/r/64422/#comment271648>
any reason those are optional?
- Jie Yu
On Dec. 7, 2017, 5:49 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64422/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2017, 5:49 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The added fields will allow us to explicitly surface resource
> provider-related information in the master.
>
>
> Diffs
> -----
>
> src/messages/messages.proto 2ab0fe8400a2de2318368d1b408b266ce647339d
>
>
> Diff: https://reviews.apache.org/r/64422/diff/1/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>