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/25 02:40:05 UTC
Review Request 55910: Prevent unintended mutation in the allocator.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55910/
-----------------------------------------------------------
Review request for mesos and Michael Park.
Repository: mesos
Description
-------
Currently, a lof of code in the allocator makes use of the `[]`
operator to access the agents, frameworks and sorters, which
can lead to subtle bugs where insertion was unintended.
With this change, a few const functions can be marked as such.
Diffs
-----
src/master/allocator/mesos/hierarchical.hpp 9b66c23f26b37c02ed850c06c4518ea99078b02d
src/master/allocator/mesos/hierarchical.cpp c2211be7458755aeb91ef078e4bfe92ac474044a
Diff: https://reviews.apache.org/r/55910/diff/
Testing
-------
make check
Thanks,
Benjamin Mahler
Re: Review Request 55910: Prevent unintended mutation in the
allocator.
Posted by Guangya Liu <gy...@gmail.com>.
> On \u4e00\u6708 27, 2017, 7:13 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 237-241
> > <https://reviews.apache.org/r/55910/diff/1/?file=1614081#file1614081line237>
> >
> > Just a nit here: It seems a bit strange to me here of getting roles as following:
> > 1) Construct `frameworks`
> > 2) Get related framework info
> > 3) Get related framework role
> >
> > How about keep the logic as before but put the logic of construct of `frameworks` to #275 here?
>
> Benjamin Mahler wrote:
> I'm not sure why it would strange to insert the framework before updating the sorters, can you clarify?
The only concern is that we can already get `role` from `frameworkInfo.role()`, but here is constructing the `frameworks` and then get `role` from the specified `framework`. My thinking is still getting roles from `frameworkInfo.role()` here and construct the `frameworks` at the end of this function. Anyway this is not a big deal, just a nit.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55910/#review163235
-----------------------------------------------------------
On \u4e00\u6708 25, 2017, 2:40 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55910/
> -----------------------------------------------------------
>
> (Updated \u4e00\u6708 25, 2017, 2:40 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Currently, a lof of code in the allocator makes use of the `[]`
> operator to access the agents, frameworks and sorters, which
> can lead to subtle bugs where insertion was unintended.
>
> With this change, a few const functions can be marked as such.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 9b66c23f26b37c02ed850c06c4518ea99078b02d
> src/master/allocator/mesos/hierarchical.cpp c2211be7458755aeb91ef078e4bfe92ac474044a
>
> Diff: https://reviews.apache.org/r/55910/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55910: Prevent unintended mutation in the
allocator.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Jan. 27, 2017, 7:13 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 237-241
> > <https://reviews.apache.org/r/55910/diff/1/?file=1614081#file1614081line237>
> >
> > Just a nit here: It seems a bit strange to me here of getting roles as following:
> > 1) Construct `frameworks`
> > 2) Get related framework info
> > 3) Get related framework role
> >
> > How about keep the logic as before but put the logic of construct of `frameworks` to #275 here?
I'm not sure why it would strange to insert the framework before updating the sorters, can you clarify?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55910/#review163235
-----------------------------------------------------------
On Jan. 25, 2017, 2:40 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55910/
> -----------------------------------------------------------
>
> (Updated Jan. 25, 2017, 2:40 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Currently, a lof of code in the allocator makes use of the `[]`
> operator to access the agents, frameworks and sorters, which
> can lead to subtle bugs where insertion was unintended.
>
> With this change, a few const functions can be marked as such.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 9b66c23f26b37c02ed850c06c4518ea99078b02d
> src/master/allocator/mesos/hierarchical.cpp c2211be7458755aeb91ef078e4bfe92ac474044a
>
> Diff: https://reviews.apache.org/r/55910/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55910: Prevent unintended mutation in the
allocator.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55910/#review163235
-----------------------------------------------------------
Fix it, then Ship it!
Ship It!
src/master/allocator/mesos/hierarchical.cpp (lines 237 - 241)
<https://reviews.apache.org/r/55910/#comment234689>
Just a nit here: It seems a bit strange to me here of getting roles as following:
1) Construct `frameworks`
2) Get related framework info
3) Get related framework role
How about keep the logic as before but put the logic of construct of `frameworks` to #275 here?
- Guangya Liu
On \u4e00\u6708 25, 2017, 2:40 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55910/
> -----------------------------------------------------------
>
> (Updated \u4e00\u6708 25, 2017, 2:40 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Currently, a lof of code in the allocator makes use of the `[]`
> operator to access the agents, frameworks and sorters, which
> can lead to subtle bugs where insertion was unintended.
>
> With this change, a few const functions can be marked as such.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 9b66c23f26b37c02ed850c06c4518ea99078b02d
> src/master/allocator/mesos/hierarchical.cpp c2211be7458755aeb91ef078e4bfe92ac474044a
>
> Diff: https://reviews.apache.org/r/55910/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55910: Prevent unintended mutation in the
allocator.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Jan. 30, 2017, 3:31 p.m., Benjamin Bannier wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 128
> > <https://reviews.apache.org/r/55910/diff/1/?file=1614081#file1614081line128>
> >
> > nit: This could be set in the member declaration.
Hm.. is that something we do? Most of our code uses the initializer list AFAICT.
> On Jan. 30, 2017, 3:31 p.m., Benjamin Bannier wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 237-241
> > <https://reviews.apache.org/r/55910/diff/1/?file=1614081#file1614081line237>
> >
> > @gyliu: I like introducing `Framework` as a helper doing extraction from a `FrameworkInfo` (this is expanded on in r/55870).
> >
> > @bmahler: `insert` also takes an `initializer_list`, i.e.,
> >
> > frameworks.insert({frameworkId, Framework(frameworkInfo)});
> >
> > Otherwise please include `<utility>`.
Will use an initializer list, thanks!
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55910/#review163522
-----------------------------------------------------------
On Jan. 25, 2017, 2:40 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55910/
> -----------------------------------------------------------
>
> (Updated Jan. 25, 2017, 2:40 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Currently, a lof of code in the allocator makes use of the `[]`
> operator to access the agents, frameworks and sorters, which
> can lead to subtle bugs where insertion was unintended.
>
> With this change, a few const functions can be marked as such.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 9b66c23f26b37c02ed850c06c4518ea99078b02d
> src/master/allocator/mesos/hierarchical.cpp c2211be7458755aeb91ef078e4bfe92ac474044a
>
> Diff: https://reviews.apache.org/r/55910/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55910: Prevent unintended mutation in the
allocator.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55910/#review163522
-----------------------------------------------------------
Fix it, then Ship it!
src/master/allocator/mesos/hierarchical.hpp (line 283)
<https://reviews.apache.org/r/55910/#comment234982>
Is this intentionally implicit? Otherwise please make this `explicit`.
src/master/allocator/mesos/hierarchical.cpp (line 128)
<https://reviews.apache.org/r/55910/#comment234983>
nit: This could be set in the member declaration.
src/master/allocator/mesos/hierarchical.cpp (lines 237 - 241)
<https://reviews.apache.org/r/55910/#comment234981>
@gyliu: I like introducing `Framework` as a helper doing extraction from a `FrameworkInfo` (this is expanded on in r/55870).
@bmahler: `insert` also takes an `initializer_list`, i.e.,
frameworks.insert({frameworkId, Framework(frameworkInfo)});
Otherwise please include `<utility>`.
- Benjamin Bannier
On Jan. 25, 2017, 3:40 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55910/
> -----------------------------------------------------------
>
> (Updated Jan. 25, 2017, 3:40 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Currently, a lof of code in the allocator makes use of the `[]`
> operator to access the agents, frameworks and sorters, which
> can lead to subtle bugs where insertion was unintended.
>
> With this change, a few const functions can be marked as such.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 9b66c23f26b37c02ed850c06c4518ea99078b02d
> src/master/allocator/mesos/hierarchical.cpp c2211be7458755aeb91ef078e4bfe92ac474044a
>
> Diff: https://reviews.apache.org/r/55910/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55910: Prevent unintended mutation in the
allocator.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Jan. 30, 2017, 8:34 p.m., Michael Park wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 387-391
> > <https://reviews.apache.org/r/55910/diff/1/?file=1614081#file1614081line387>
> >
> > Not yours, but do you know what's going on here?
> > The comment doesn't seem to match the code.
Hm.. did you interpret 'delete' to mean clear? The 'delete' is deferred until expire.
> On Jan. 30, 2017, 8:34 p.m., Michael Park wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1446
> > <https://reviews.apache.org/r/55910/diff/1/?file=1614081#file1614081line1446>
> >
> > Did you mean `const Owned<Sorter>&` here?
> >
> > I'd prefer to not introduce more places where `Owned` is being used like a `Shared`.
> >
> > or maybe even:
> >
> > ```cpp
> > Sorter& frameworkSorter = *frameworkSorters.at(role);
> > ```
Whoops, yes I meant `const Owned<Sorter>&` as done on the other lookups.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55910/#review163567
-----------------------------------------------------------
On Jan. 25, 2017, 2:40 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55910/
> -----------------------------------------------------------
>
> (Updated Jan. 25, 2017, 2:40 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Currently, a lof of code in the allocator makes use of the `[]`
> operator to access the agents, frameworks and sorters, which
> can lead to subtle bugs where insertion was unintended.
>
> With this change, a few const functions can be marked as such.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 9b66c23f26b37c02ed850c06c4518ea99078b02d
> src/master/allocator/mesos/hierarchical.cpp c2211be7458755aeb91ef078e4bfe92ac474044a
>
> Diff: https://reviews.apache.org/r/55910/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55910: Prevent unintended mutation in the
allocator.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55910/#review163567
-----------------------------------------------------------
Fix it, then Ship it!
src/master/allocator/mesos/hierarchical.cpp (lines 381 - 385)
<https://reviews.apache.org/r/55910/#comment235018>
Not yours, but do you know what's going on here?
The comment doesn't seem to match the code.
src/master/allocator/mesos/hierarchical.cpp (line 632)
<https://reviews.apache.org/r/55910/#comment235021>
Seems like we could've just kept:
```cpp
const string& role = frameworks.at(frameworkId).role;
```
and used `role` rather than `framework.role` below.
src/master/allocator/mesos/hierarchical.cpp (line 1424)
<https://reviews.apache.org/r/55910/#comment235022>
Did you mean `const Owned<Sorter>&` here?
I'd prefer to not introduce more places where `Owned` is being used like a `Shared`.
or maybe even:
```cpp
Sorter& frameworkSorter = *frameworkSorters.at(role);
```
- Michael Park
On Jan. 24, 2017, 6:40 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55910/
> -----------------------------------------------------------
>
> (Updated Jan. 24, 2017, 6:40 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Currently, a lof of code in the allocator makes use of the `[]`
> operator to access the agents, frameworks and sorters, which
> can lead to subtle bugs where insertion was unintended.
>
> With this change, a few const functions can be marked as such.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 9b66c23f26b37c02ed850c06c4518ea99078b02d
> src/master/allocator/mesos/hierarchical.cpp c2211be7458755aeb91ef078e4bfe92ac474044a
>
> Diff: https://reviews.apache.org/r/55910/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 55910: Prevent unintended mutation in the
allocator.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55910/#review162998
-----------------------------------------------------------
Patch looks great!
Reviews applied: [54842, 54836, 55825, 55826, 55827, 55829, 55828, 55863, 55866, 55824, 55868, 55908, 55909, 55910]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Jan. 25, 2017, 2:40 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55910/
> -----------------------------------------------------------
>
> (Updated Jan. 25, 2017, 2:40 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Currently, a lof of code in the allocator makes use of the `[]`
> operator to access the agents, frameworks and sorters, which
> can lead to subtle bugs where insertion was unintended.
>
> With this change, a few const functions can be marked as such.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 9b66c23f26b37c02ed850c06c4518ea99078b02d
> src/master/allocator/mesos/hierarchical.cpp c2211be7458755aeb91ef078e4bfe92ac474044a
>
> Diff: https://reviews.apache.org/r/55910/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin Mahler
>
>