You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benno Evers <be...@mesosphere.com> on 2019/10/29 18:14:25 UTC
Review Request 71695: Updated 'Master::Http::_reserve' to pass along
new 'source' field.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71695/
-----------------------------------------------------------
Review request for mesos and Benjamin Bannier.
Bugs: MESOS-9987 and MESOS-9989
https://issues.apache.org/jira/browse/MESOS-9987
https://issues.apache.org/jira/browse/MESOS-9989
Repository: mesos
Description
-------
Updated 'Master::Http::_reserve()' to correctly set the new `source`
field in the `Offer::Operation` created from operator API input.
Diffs
-----
src/master/http.cpp 60765c9b9d6903f6ed94fa8c614055698caad0da
src/master/master.hpp dc45028d2ecfb61bf9ea82d90d2393af648a6023
Diff: https://reviews.apache.org/r/71695/diff/1/
Testing
-------
Thanks,
Benno Evers
Re: Review Request 71695: Updated 'Master::Http::_reserve' to pass
along new 'source' field.
Posted by Benjamin Bannier <bb...@apache.org>.
> On Nov. 6, 2019, 2:35 a.m., Greg Mann wrote:
> > src/master/http.cpp
> > Lines 1988 (patched)
> > <https://reviews.apache.org/r/71695/diff/2/?file=2171533#file2171533line1994>
> >
> > Should this be an Option? Since it isn't, it seems like we will set the `source` field to a default-constructed value in `_reserve()` when the operator does not specify that parameter. Would it be better to avoid setting the `source` field at all in the offer operation if it isn't found in the query parameters?
>
> Benno Evers wrote:
> As far as I know, there's no difference in the protobuf wire format between an unset and a zero-length repeated field. So as soon as we have constructed the `Reserve` protobuf inside `_reserve()`, we have no way to distinguish these cases anyways.
>
> So the only question seems to be how early we want to forget about whether or not `source` was specified, and I opted to do it here for symmetry with how we pass `resources`.
I agree with Benno here. Additionally, we are merely unpacking the protobuf data here, and want to make decisions based on empty/non-empty state elsewhere (which for `repeated` translates to set/unset). I cannot see what additional wrapping in `Option` would add.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71695/#review218519
-----------------------------------------------------------
On Nov. 5, 2019, 2:25 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71695/
> -----------------------------------------------------------
>
> (Updated Nov. 5, 2019, 2:25 p.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-9987 and MESOS-9989
> https://issues.apache.org/jira/browse/MESOS-9987
> https://issues.apache.org/jira/browse/MESOS-9989
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated 'Master::Http::_reserve()' to correctly set the new `source`
> field in the `Offer::Operation` created from operator API input.
>
>
> Diffs
> -----
>
> src/master/http.cpp 60765c9b9d6903f6ed94fa8c614055698caad0da
> src/master/master.hpp dc45028d2ecfb61bf9ea82d90d2393af648a6023
>
>
> Diff: https://reviews.apache.org/r/71695/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 71695: Updated 'Master::Http::_reserve' to pass
along new 'source' field.
Posted by Benno Evers <be...@mesosphere.com>.
> On Nov. 6, 2019, 1:35 a.m., Greg Mann wrote:
> > src/master/http.cpp
> > Lines 1988 (patched)
> > <https://reviews.apache.org/r/71695/diff/2/?file=2171533#file2171533line1994>
> >
> > Should this be an Option? Since it isn't, it seems like we will set the `source` field to a default-constructed value in `_reserve()` when the operator does not specify that parameter. Would it be better to avoid setting the `source` field at all in the offer operation if it isn't found in the query parameters?
As far as I know, there's no difference in the protobuf wire format between an unset and a zero-length repeated field. So as soon as we have constructed the `Reserve` protobuf inside `_reserve()`, we have no way to distinguish these cases anyways.
So the only question seems to be how early we want to forget about whether or not `source` was specified, and I opted to do it here for symmetry with how we pass `resources`.
- Benno
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71695/#review218519
-----------------------------------------------------------
On Nov. 5, 2019, 1:25 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71695/
> -----------------------------------------------------------
>
> (Updated Nov. 5, 2019, 1:25 p.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-9987 and MESOS-9989
> https://issues.apache.org/jira/browse/MESOS-9987
> https://issues.apache.org/jira/browse/MESOS-9989
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated 'Master::Http::_reserve()' to correctly set the new `source`
> field in the `Offer::Operation` created from operator API input.
>
>
> Diffs
> -----
>
> src/master/http.cpp 60765c9b9d6903f6ed94fa8c614055698caad0da
> src/master/master.hpp dc45028d2ecfb61bf9ea82d90d2393af648a6023
>
>
> Diff: https://reviews.apache.org/r/71695/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 71695: Updated 'Master::Http::_reserve' to pass
along new 'source' field.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71695/#review218519
-----------------------------------------------------------
src/master/http.cpp
Lines 1988 (patched)
<https://reviews.apache.org/r/71695/#comment306262>
Should this be an Option? Since it isn't, it seems like we will set the `source` field to a default-constructed value in `_reserve()` when the operator does not specify that parameter. Would it be better to avoid setting the `source` field at all in the offer operation if it isn't found in the query parameters?
- Greg Mann
On Nov. 5, 2019, 1:25 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71695/
> -----------------------------------------------------------
>
> (Updated Nov. 5, 2019, 1:25 p.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-9987 and MESOS-9989
> https://issues.apache.org/jira/browse/MESOS-9987
> https://issues.apache.org/jira/browse/MESOS-9989
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated 'Master::Http::_reserve()' to correctly set the new `source`
> field in the `Offer::Operation` created from operator API input.
>
>
> Diffs
> -----
>
> src/master/http.cpp 60765c9b9d6903f6ed94fa8c614055698caad0da
> src/master/master.hpp dc45028d2ecfb61bf9ea82d90d2393af648a6023
>
>
> Diff: https://reviews.apache.org/r/71695/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 71695: Updated 'Master::Http::_reserve' to pass
along new 'source' field.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71695/#review218541
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Bannier
On Nov. 5, 2019, 2:25 p.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71695/
> -----------------------------------------------------------
>
> (Updated Nov. 5, 2019, 2:25 p.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-9987 and MESOS-9989
> https://issues.apache.org/jira/browse/MESOS-9987
> https://issues.apache.org/jira/browse/MESOS-9989
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Updated 'Master::Http::_reserve()' to correctly set the new `source`
> field in the `Offer::Operation` created from operator API input.
>
>
> Diffs
> -----
>
> src/master/http.cpp 60765c9b9d6903f6ed94fa8c614055698caad0da
> src/master/master.hpp dc45028d2ecfb61bf9ea82d90d2393af648a6023
>
>
> Diff: https://reviews.apache.org/r/71695/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 71695: Updated 'Master::Http::_reserve' to pass
along new 'source' field.
Posted by Benno Evers <be...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71695/
-----------------------------------------------------------
(Updated Nov. 5, 2019, 1:25 p.m.)
Review request for mesos and Benjamin Bannier.
Changes
-------
Incorporated fixups by @bbannier
Bugs: MESOS-9987 and MESOS-9989
https://issues.apache.org/jira/browse/MESOS-9987
https://issues.apache.org/jira/browse/MESOS-9989
Repository: mesos
Description
-------
Updated 'Master::Http::_reserve()' to correctly set the new `source`
field in the `Offer::Operation` created from operator API input.
Diffs (updated)
-----
src/master/http.cpp 60765c9b9d6903f6ed94fa8c614055698caad0da
src/master/master.hpp dc45028d2ecfb61bf9ea82d90d2393af648a6023
Diff: https://reviews.apache.org/r/71695/diff/2/
Changes: https://reviews.apache.org/r/71695/diff/1-2/
Testing
-------
Thanks,
Benno Evers