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 Mahler <bm...@apache.org> on 2017/01/27 00:30:26 UTC
Review Request 56004: Fixed MULTI_ROLE related bugs when updating
framework info.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56004/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier and Michael Park.
Repository: mesos
Description
-------
The first issue is that we need to update the capabilities member
to reflect the new capabilities.
The second issue is that when we allow an upgrade or downgrade
to or from MULTI_ROLE, we need to update the `role` and `roles`
fields of `FrameworkInfo`.
Diffs
-----
src/master/master.hpp 7e38af41ca16241dbbe3bc2e80c0848e82762a45
Diff: https://reviews.apache.org/r/56004/diff/
Testing
-------
make check
Thanks,
Benjamin Mahler
Re: Review Request 56004: Fixed MULTI_ROLE related bugs when updating
framework info.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Jan. 30, 2017, 10:26 a.m., Benjamin Bannier wrote:
> > src/master/master.hpp, line 2448
> > <https://reviews.apache.org/r/56004/diff/1/?file=1617175#file1617175line2448>
> >
> > Not yours, but should likely be
> >
> > if (source.capabilities.empty()) {
> > info.clear_capabilities();
> > } else {
> > info.mutable_capabilities()->CopyFrom(source.capabilities());
> > }
Hm.. are you referring to the capabilities block below using empty instead of size? I suppose that piece could even just be:
```
info.mutable_capabilities()->CopyFrom(source.capabilities());
```
Since it's a repeated field.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56004/#review163488
-----------------------------------------------------------
On Jan. 27, 2017, 12:30 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56004/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2017, 12:30 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The first issue is that we need to update the capabilities member
> to reflect the new capabilities.
>
> The second issue is that when we allow an upgrade or downgrade
> to or from MULTI_ROLE, we need to update the `role` and `roles`
> fields of `FrameworkInfo`.
>
>
> Diffs
> -----
>
> src/master/master.hpp 7e38af41ca16241dbbe3bc2e80c0848e82762a45
>
> Diff: https://reviews.apache.org/r/56004/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 56004: Fixed MULTI_ROLE related bugs when updating
framework info.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56004/#review163488
-----------------------------------------------------------
src/master/master.hpp (line 2438)
<https://reviews.apache.org/r/56004/#comment234961>
Not yours, but should likely be
if (source.capabilities.empty()) {
info.clear_capabilities();
} else {
info.mutable_capabilities()->CopyFrom(source.capabilities());
}
- Benjamin Bannier
On Jan. 27, 2017, 1:30 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56004/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2017, 1:30 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The first issue is that we need to update the capabilities member
> to reflect the new capabilities.
>
> The second issue is that when we allow an upgrade or downgrade
> to or from MULTI_ROLE, we need to update the `role` and `roles`
> fields of `FrameworkInfo`.
>
>
> Diffs
> -----
>
> src/master/master.hpp 7e38af41ca16241dbbe3bc2e80c0848e82762a45
>
> Diff: https://reviews.apache.org/r/56004/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 56004: Fixed MULTI_ROLE related bugs when updating
framework info.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Feb. 1, 2017, 11:38 p.m., Michael Park wrote:
> > src/master/master.hpp, lines 2441-2450
> > <https://reviews.apache.org/r/56004/diff/1/?file=1617175#file1617175line2441>
> >
> > Could we just follow this pattern?
> >
> > ```cpp
> > if (source.has_role()) {
> > info.set_role(source.role());
> > } else {
> > info.clear_role();
> > }
> >
> > if (source.roles_size() > 0) {
> > info.mutable_roles()->CopyFrom(source.roles());
> > } else {
> > info.clear_roles();
> > }
> > ```
I found this harder to follow. The one I went with was: wipe the fields and then set based on source. The suggestion here seems to be: if the source has it set, overwrite, otherwise wipe, which just seemed a little less clear to me. Thoughts?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56004/#review163900
-----------------------------------------------------------
On Jan. 27, 2017, 12:30 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56004/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2017, 12:30 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The first issue is that we need to update the capabilities member
> to reflect the new capabilities.
>
> The second issue is that when we allow an upgrade or downgrade
> to or from MULTI_ROLE, we need to update the `role` and `roles`
> fields of `FrameworkInfo`.
>
>
> Diffs
> -----
>
> src/master/master.hpp 7e38af41ca16241dbbe3bc2e80c0848e82762a45
>
> Diff: https://reviews.apache.org/r/56004/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 56004: Fixed MULTI_ROLE related bugs when updating
framework info.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56004/#review163900
-----------------------------------------------------------
Ship it!
Thanks for catching these issues!
src/master/master.hpp (lines 2431 - 2440)
<https://reviews.apache.org/r/56004/#comment235411>
Could we just follow this pattern?
```cpp
if (source.has_role()) {
info.set_role(source.role());
} else {
info.clear_role();
}
if (source.roles_size() > 0) {
info.mutable_roles()->CopyFrom(source.roles());
} else {
info.clear_roles();
}
```
- Michael Park
On Jan. 26, 2017, 4:30 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56004/
> -----------------------------------------------------------
>
> (Updated Jan. 26, 2017, 4:30 p.m.)
>
>
> Review request for mesos, Benjamin Bannier and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The first issue is that we need to update the capabilities member
> to reflect the new capabilities.
>
> The second issue is that when we allow an upgrade or downgrade
> to or from MULTI_ROLE, we need to update the `role` and `roles`
> fields of `FrameworkInfo`.
>
>
> Diffs
> -----
>
> src/master/master.hpp 7e38af41ca16241dbbe3bc2e80c0848e82762a45
>
> Diff: https://reviews.apache.org/r/56004/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 56004: Fixed MULTI_ROLE related bugs when updating
framework info.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Jan. 30, 2017, 1:46 a.m., Guangya Liu wrote:
> > src/master/master.hpp, lines 2441-2450
> > <https://reviews.apache.org/r/56004/diff/1/?file=1617175#file1617175line2441>
> >
> > Can you please add some comments here to clarify that this is used to handle upgrade/downgrade case to/from multi role framework?
>
> Benjamin Bannier wrote:
> This whole block (and also the added code) are related to any changes in framework capabilities, right?
Yeah I would say that this is captured by the comments that are already present, no?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56004/#review163456
-----------------------------------------------------------
On Jan. 27, 2017, 12:30 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56004/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2017, 12:30 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The first issue is that we need to update the capabilities member
> to reflect the new capabilities.
>
> The second issue is that when we allow an upgrade or downgrade
> to or from MULTI_ROLE, we need to update the `role` and `roles`
> fields of `FrameworkInfo`.
>
>
> Diffs
> -----
>
> src/master/master.hpp 7e38af41ca16241dbbe3bc2e80c0848e82762a45
>
> Diff: https://reviews.apache.org/r/56004/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 56004: Fixed MULTI_ROLE related bugs when updating
framework info.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Jan. 30, 2017, 2:46 a.m., Guangya Liu wrote:
> > src/master/master.hpp, lines 2441-2450
> > <https://reviews.apache.org/r/56004/diff/1/?file=1617175#file1617175line2441>
> >
> > Can you please add some comments here to clarify that this is used to handle upgrade/downgrade case to/from multi role framework?
This whole block (and also the added code) are related to any changes in framework capabilities, right?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56004/#review163456
-----------------------------------------------------------
On Jan. 27, 2017, 1:30 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56004/
> -----------------------------------------------------------
>
> (Updated Jan. 27, 2017, 1:30 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The first issue is that we need to update the capabilities member
> to reflect the new capabilities.
>
> The second issue is that when we allow an upgrade or downgrade
> to or from MULTI_ROLE, we need to update the `role` and `roles`
> fields of `FrameworkInfo`.
>
>
> Diffs
> -----
>
> src/master/master.hpp 7e38af41ca16241dbbe3bc2e80c0848e82762a45
>
> Diff: https://reviews.apache.org/r/56004/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 56004: Fixed MULTI_ROLE related bugs when updating
framework info.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56004/#review163456
-----------------------------------------------------------
Fix it, then Ship it!
Ship It!
src/master/master.hpp (lines 2431 - 2440)
<https://reviews.apache.org/r/56004/#comment234917>
Can you please add some comments here to clarify that this is used to handle upgrade/downgrade case to/from multi role framework?
- Guangya Liu
On \u4e00\u6708 27, 2017, 12:30 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56004/
> -----------------------------------------------------------
>
> (Updated \u4e00\u6708 27, 2017, 12:30 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The first issue is that we need to update the capabilities member
> to reflect the new capabilities.
>
> The second issue is that when we allow an upgrade or downgrade
> to or from MULTI_ROLE, we need to update the `role` and `roles`
> fields of `FrameworkInfo`.
>
>
> Diffs
> -----
>
> src/master/master.hpp 7e38af41ca16241dbbe3bc2e80c0848e82762a45
>
> Diff: https://reviews.apache.org/r/56004/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>