You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jay Guo <gu...@gmail.com> on 2016/12/12 08:42:34 UTC
Review Request 54650: Added validation for roles in ACCEPT call.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54650/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
Bugs: MESOS-6637
https://issues.apache.org/jira/browse/MESOS-6637
Repository: mesos
Description
-------
For a multi-role framework, it may receive offers for different roles
in that framework. However, we disallow multiple offers with different
roles being accepted in single ACCEPT call.
Diffs
-----
src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f
Diff: https://reviews.apache.org/r/54650/diff/
Testing
-------
make check
Thanks,
Jay Guo
Re: Review Request 54650: Added validation for roles in ACCEPT call.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54650/#review158897
-----------------------------------------------------------
Patch looks great!
Reviews applied: [54649, 54650]
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 Dec. 12, 2016, 2:30 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54650/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2016, 2:30 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For a multi-role framework, it may receive offers for different roles
> in that framework. However, we disallow multiple offers with different
> roles being accepted in single ACCEPT call.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f
>
> Diff: https://reviews.apache.org/r/54650/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 54650: Added validation for roles in ACCEPT call.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54650/#review164393
-----------------------------------------------------------
Fix it, then Ship it!
There seems to be a bug, otherwise looks good.
Feel free to split out the offer id printing fix.
src/master/validation.cpp (line 1308)
<https://reviews.apache.org/r/54650/#comment236083>
If you want to pull these two consistency fixes out I could get that committed for you.
src/master/validation.cpp (line 1401)
<https://reviews.apache.org/r/54650/#comment236085>
How about validateAllocationRole?
src/master/validation.cpp (line 1415)
<https://reviews.apache.org/r/54650/#comment236084>
This line looks wrong? You could replace it with a 'continue' but it's also fine if you just remove it.
- Benjamin Mahler
On Dec. 13, 2016, 4:41 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54650/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2016, 4:41 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For a multi-role framework, it may receive offers for different roles
> in that framework. However, we disallow multiple offers with different
> roles being accepted in single ACCEPT call.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f
>
> Diff: https://reviews.apache.org/r/54650/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 54650: Added validation for roles in ACCEPT call.
Posted by Jay Guo <gu...@gmail.com>.
> On Dec. 13, 2016, 11:04 p.m., Guangya Liu wrote:
> > src/master/validation.cpp, line 1411
> > <https://reviews.apache.org/r/54650/diff/3/?file=1582390#file1582390line1411>
> >
> > Shall we add some check here to make sure the `allocation_info` exist as this field is `optional`?
>
> Benjamin Mahler wrote:
> That sounds good to me, note that it means we can't land this patch until we land my allocator / master changes.
will change accordingly when that's done.
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54650/#review159009
-----------------------------------------------------------
On Dec. 13, 2016, 12:41 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54650/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2016, 12:41 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For a multi-role framework, it may receive offers for different roles
> in that framework. However, we disallow multiple offers with different
> roles being accepted in single ACCEPT call.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f
>
> Diff: https://reviews.apache.org/r/54650/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 54650: Added validation for roles in ACCEPT call.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Dec. 13, 2016, 4:04 p.m., Guangya Liu wrote:
> > src/master/validation.cpp, line 1411
> > <https://reviews.apache.org/r/54650/diff/3/?file=1582390#file1582390line1411>
> >
> > Shall we add some check here to make sure the `allocation_info` exist as this field is `optional`?
>
> Benjamin Mahler wrote:
> That sounds good to me, note that it means we can't land this patch until we land my allocator / master changes.
>
> Jay Guo wrote:
> will change accordingly when that's done.
Note that the current implementation will also work if one mixed allocated and not allocated resources (`_role` would just be an empty string for unallocated resources). I think we should continue handling that edge case.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54650/#review159009
-----------------------------------------------------------
On Dec. 13, 2016, 5:41 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54650/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2016, 5:41 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For a multi-role framework, it may receive offers for different roles
> in that framework. However, we disallow multiple offers with different
> roles being accepted in single ACCEPT call.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f
>
> Diff: https://reviews.apache.org/r/54650/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 54650: Added validation for roles in ACCEPT call.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Dec. 13, 2016, 3:04 p.m., Guangya Liu wrote:
> > src/master/validation.cpp, line 1411
> > <https://reviews.apache.org/r/54650/diff/3/?file=1582390#file1582390line1411>
> >
> > Shall we add some check here to make sure the `allocation_info` exist as this field is `optional`?
That sounds good to me, note that it means we can't land this patch until we land my allocator / master changes.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54650/#review159009
-----------------------------------------------------------
On Dec. 13, 2016, 4:41 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54650/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2016, 4:41 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For a multi-role framework, it may receive offers for different roles
> in that framework. However, we disallow multiple offers with different
> roles being accepted in single ACCEPT call.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f
>
> Diff: https://reviews.apache.org/r/54650/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 54650: Added validation for roles in ACCEPT call.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54650/#review159009
-----------------------------------------------------------
src/master/validation.cpp (line 1411)
<https://reviews.apache.org/r/54650/#comment229932>
Shall we add some check here to make sure the `allocation_info` exist as this field is `optional`?
- Guangya Liu
On \u5341\u4e8c\u6708 13, 2016, 4:41 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54650/
> -----------------------------------------------------------
>
> (Updated \u5341\u4e8c\u6708 13, 2016, 4:41 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For a multi-role framework, it may receive offers for different roles
> in that framework. However, we disallow multiple offers with different
> roles being accepted in single ACCEPT call.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f
>
> Diff: https://reviews.apache.org/r/54650/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 54650: Added validation for roles in ACCEPT call.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54650/#review159107
-----------------------------------------------------------
Patch looks great!
Reviews applied: [54649, 54650]
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 Dec. 13, 2016, 4:41 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54650/
> -----------------------------------------------------------
>
> (Updated Dec. 13, 2016, 4:41 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For a multi-role framework, it may receive offers for different roles
> in that framework. However, we disallow multiple offers with different
> roles being accepted in single ACCEPT call.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f
>
> Diff: https://reviews.apache.org/r/54650/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 54650: Added validation for roles in ACCEPT call.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54650/#review164466
-----------------------------------------------------------
Ship it!
src/master/validation.cpp (line 1462)
<https://reviews.apache.org/r/54650/#comment236205>
I created https://issues.apache.org/jira/browse/MESOS-7072 to clean up the use of `T*` for in parameters here.
src/master/validation.cpp (line 1473)
<https://reviews.apache.org/r/54650/#comment236204>
Small nit: `role()` returns a `const string&`, and we just store the result in `_role` to have a convenient short-hand. If we'd bind the result of `role()` to a `const string&` instead of creating a copy we could avoid needless creation of strings for all but the first element (where we store a copy in `role`).
- Benjamin Bannier
On Feb. 7, 2017, 3:10 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54650/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2017, 3:10 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For a multi-role framework, it may receive offers for different roles
> in that framework. However, we disallow multiple offers with different
> roles being accepted in single ACCEPT call.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 2beee167439fe39d4f595c6b858fb6175321fbcd
>
> Diff: https://reviews.apache.org/r/54650/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 54650: Added validation for roles in ACCEPT call.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54650/#review164790
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Mahler
On Feb. 7, 2017, 7:34 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54650/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2017, 7:34 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For a multi-role framework, it may receive offers for different roles
> in that framework. However, we disallow multiple offers with different
> roles being accepted in single ACCEPT call.
>
>
> Diffs
> -----
>
> src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a
> src/master/validation.cpp 2beee167439fe39d4f595c6b858fb6175321fbcd
>
> Diff: https://reviews.apache.org/r/54650/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 54650: Added validation for roles in ACCEPT call.
Posted by Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54650/
-----------------------------------------------------------
(Updated Feb. 7, 2017, 3:34 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
Changes
-------
remove TODO
Bugs: MESOS-6637
https://issues.apache.org/jira/browse/MESOS-6637
Repository: mesos
Description
-------
For a multi-role framework, it may receive offers for different roles
in that framework. However, we disallow multiple offers with different
roles being accepted in single ACCEPT call.
Diffs (updated)
-----
src/master/master.cpp 98c39b279e7b9830d02efc8ec6a4469afc15d62a
src/master/validation.cpp 2beee167439fe39d4f595c6b858fb6175321fbcd
Diff: https://reviews.apache.org/r/54650/diff/
Testing
-------
make check
Thanks,
Jay Guo
Re: Review Request 54650: Added validation for roles in ACCEPT call.
Posted by Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54650/
-----------------------------------------------------------
(Updated Feb. 7, 2017, 10:10 a.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
Changes
-------
rebase and address comments from BenM and Guangya
Bugs: MESOS-6637
https://issues.apache.org/jira/browse/MESOS-6637
Repository: mesos
Description
-------
For a multi-role framework, it may receive offers for different roles
in that framework. However, we disallow multiple offers with different
roles being accepted in single ACCEPT call.
Diffs (updated)
-----
src/master/validation.cpp 2beee167439fe39d4f595c6b858fb6175321fbcd
Diff: https://reviews.apache.org/r/54650/diff/
Testing
-------
make check
Thanks,
Jay Guo
Re: Review Request 54650: Added validation for roles in ACCEPT call.
Posted by Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54650/
-----------------------------------------------------------
(Updated Dec. 13, 2016, 12:41 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
Changes
-------
addressed part of Qian's and Ben's comments.
Bugs: MESOS-6637
https://issues.apache.org/jira/browse/MESOS-6637
Repository: mesos
Description
-------
For a multi-role framework, it may receive offers for different roles
in that framework. However, we disallow multiple offers with different
roles being accepted in single ACCEPT call.
Diffs (updated)
-----
src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f
Diff: https://reviews.apache.org/r/54650/diff/
Testing
-------
make check
Thanks,
Jay Guo
Re: Review Request 54650: Added validation for roles in ACCEPT call.
Posted by Jay Guo <gu...@gmail.com>.
> On Dec. 13, 2016, 8:40 a.m., Benjamin Mahler wrote:
> > src/master/validation.cpp, line 1425
> > <https://reviews.apache.org/r/54650/diff/2/?file=1581993#file1581993line1425>
> >
> > This return here looks incorrect? Maybe copy over a similar comment from validateSlave:
> >
> > ```
> > // Use the first role to validate against the rest.
> > if (role.isNone()) {
> > role = _role.get();
> > }
> > ```
hmm... why do we want to continue following validation if we hit this base case? Next check would pass would be the same anyway since we have just set them to be identical. I think we could return `None()` for `validateSlave` as well...
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54650/#review158937
-----------------------------------------------------------
On Dec. 12, 2016, 10:30 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54650/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2016, 10:30 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For a multi-role framework, it may receive offers for different roles
> in that framework. However, we disallow multiple offers with different
> roles being accepted in single ACCEPT call.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f
>
> Diff: https://reviews.apache.org/r/54650/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 54650: Added validation for roles in ACCEPT call.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54650/#review158937
-----------------------------------------------------------
src/master/validation.cpp (lines 1329 - 1337)
<https://reviews.apache.org/r/54650/#comment229778>
Can you inline this instead of pulling out a function? It would be nice to use compose together the functions we need:
-> get the offer
-> handle no offer
-> get the role
-> ...
We tend to avoid functions that bundle together independent operations (in this case: "get the offer, and if the role is available get the role). (Also note that getRole might be a bit ambiguous in that there are "allocated" roles, and "reservation" roles).
src/master/validation.cpp (line 1425)
<https://reviews.apache.org/r/54650/#comment229779>
This return here looks incorrect? Maybe copy over a similar comment from validateSlave:
```
// Use the first role to validate against the rest.
if (role.isNone()) {
role = _role.get();
}
```
src/master/validation.cpp (lines 1428 - 1431)
<https://reviews.apache.org/r/54650/#comment229781>
Can you make this message similar to the one we use in validateSlave?
```
Aggregated offers must be allocated to the same role; Offer <ID1> is allocated to role 'r1' but another is allocated to role 'r2'
```
- Benjamin Mahler
On Dec. 12, 2016, 2:30 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54650/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2016, 2:30 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For a multi-role framework, it may receive offers for different roles
> in that framework. However, we disallow multiple offers with different
> roles being accepted in single ACCEPT call.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f
>
> Diff: https://reviews.apache.org/r/54650/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 54650: Added validation for roles in ACCEPT call.
Posted by Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54650/
-----------------------------------------------------------
(Updated Dec. 12, 2016, 10:30 p.m.)
Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
Changes
-------
addressed Qian's comments.
Bugs: MESOS-6637
https://issues.apache.org/jira/browse/MESOS-6637
Repository: mesos
Description
-------
For a multi-role framework, it may receive offers for different roles
in that framework. However, we disallow multiple offers with different
roles being accepted in single ACCEPT call.
Diffs (updated)
-----
src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f
Diff: https://reviews.apache.org/r/54650/diff/
Testing
-------
make check
Thanks,
Jay Guo
Re: Review Request 54650: Added validation for roles in ACCEPT call.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54650/#review158835
-----------------------------------------------------------
Patch looks great!
Reviews applied: [54649, 54650]
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 Dec. 12, 2016, 8:42 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54650/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2016, 8:42 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For a multi-role framework, it may receive offers for different roles
> in that framework. However, we disallow multiple offers with different
> roles being accepted in single ACCEPT call.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f
>
> Diff: https://reviews.apache.org/r/54650/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 54650: Added validation for roles in ACCEPT call.
Posted by Jay Guo <gu...@gmail.com>.
> On Dec. 12, 2016, 9:41 p.m., Qian Zhang wrote:
> > src/master/validation.cpp, line 1337
> > <https://reviews.apache.org/r/54650/diff/1/?file=1581654#file1581654line1337>
> >
> > I would suggest to change to:
> > ```
> > return Error("Offer " + stringify(offerId) + " is no longer valid");
> > ```
We tend to put caller available info in their own error message instead of callee's, see https://reviews.apache.org/r/54062/ for detailed explaination from BenM.
> On Dec. 12, 2016, 9:41 p.m., Qian Zhang wrote:
> > src/master/validation.cpp, lines 1425-1426
> > <https://reviews.apache.org/r/54650/diff/1/?file=1581654#file1581654line1425>
> >
> > Suggest to change to:
> > ```
> > return Error("Offers for different roles in offer list);
> > ```
> > Similar with the error message in `validateUniqueOfferID()`.
Could you elaborate on the message?
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54650/#review158842
-----------------------------------------------------------
On Dec. 12, 2016, 4:42 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54650/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2016, 4:42 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For a multi-role framework, it may receive offers for different roles
> in that framework. However, we disallow multiple offers with different
> roles being accepted in single ACCEPT call.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f
>
> Diff: https://reviews.apache.org/r/54650/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 54650: Added validation for roles in ACCEPT call.
Posted by Jay Guo <gu...@gmail.com>.
> On Dec. 12, 2016, 9:41 p.m., Qian Zhang wrote:
> > src/master/validation.cpp, line 1337
> > <https://reviews.apache.org/r/54650/diff/1/?file=1581654#file1581654line1337>
> >
> > I would suggest to change to:
> > ```
> > return Error("Offer " + stringify(offerId) + " is no longer valid");
> > ```
>
> Jay Guo wrote:
> We tend to put caller available info in their own error message instead of callee's, see https://reviews.apache.org/r/54062/ for detailed explaination from BenM.
>
> Benjamin Mahler wrote:
> Jay: that would be the general rule we follow currently, but I'm not sure we follow this during validation (due to the way the calling code is written). We should make sure the final validation error message that we'll log or return to the framework includes the offer id. Does it?
IINM, OfferId is printed here: https://github.com/apache/mesos/blob/master/src/master/master.cpp#L3530
- Jay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54650/#review158842
-----------------------------------------------------------
On Dec. 12, 2016, 10:30 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54650/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2016, 10:30 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For a multi-role framework, it may receive offers for different roles
> in that framework. However, we disallow multiple offers with different
> roles being accepted in single ACCEPT call.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f
>
> Diff: https://reviews.apache.org/r/54650/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 54650: Added validation for roles in ACCEPT call.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Dec. 12, 2016, 1:41 p.m., Qian Zhang wrote:
> > src/master/validation.cpp, line 1337
> > <https://reviews.apache.org/r/54650/diff/1/?file=1581654#file1581654line1337>
> >
> > I would suggest to change to:
> > ```
> > return Error("Offer " + stringify(offerId) + " is no longer valid");
> > ```
>
> Jay Guo wrote:
> We tend to put caller available info in their own error message instead of callee's, see https://reviews.apache.org/r/54062/ for detailed explaination from BenM.
Jay: that would be the general rule we follow currently, but I'm not sure we follow this during validation (due to the way the calling code is written). We should make sure the final validation error message that we'll log or return to the framework includes the offer id. Does it?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54650/#review158842
-----------------------------------------------------------
On Dec. 12, 2016, 2:30 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54650/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2016, 2:30 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For a multi-role framework, it may receive offers for different roles
> in that framework. However, we disallow multiple offers with different
> roles being accepted in single ACCEPT call.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f
>
> Diff: https://reviews.apache.org/r/54650/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 54650: Added validation for roles in ACCEPT call.
Posted by Qian Zhang <zh...@gmail.com>.
> On Dec. 12, 2016, 9:41 p.m., Qian Zhang wrote:
> > src/master/validation.cpp, line 1337
> > <https://reviews.apache.org/r/54650/diff/1/?file=1581654#file1581654line1337>
> >
> > I would suggest to change to:
> > ```
> > return Error("Offer " + stringify(offerId) + " is no longer valid");
> > ```
>
> Jay Guo wrote:
> We tend to put caller available info in their own error message instead of callee's, see https://reviews.apache.org/r/54062/ for detailed explaination from BenM.
>
> Benjamin Mahler wrote:
> Jay: that would be the general rule we follow currently, but I'm not sure we follow this during validation (due to the way the calling code is written). We should make sure the final validation error message that we'll log or return to the framework includes the offer id. Does it?
>
> Jay Guo wrote:
> IINM, OfferId is printed here: https://github.com/apache/mesos/blob/master/src/master/master.cpp#L3530
That is just the warning message we log in master side, what about the error message that we will return to framework? I do not think OfferId is included currently: https://github.com/apache/mesos/blob/master/src/master/master.cpp#L3562
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54650/#review158842
-----------------------------------------------------------
On Dec. 12, 2016, 10:30 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54650/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2016, 10:30 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For a multi-role framework, it may receive offers for different roles
> in that framework. However, we disallow multiple offers with different
> roles being accepted in single ACCEPT call.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f
>
> Diff: https://reviews.apache.org/r/54650/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 54650: Added validation for roles in ACCEPT call.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54650/#review158842
-----------------------------------------------------------
src/master/validation.cpp (line 1331)
<https://reviews.apache.org/r/54650/#comment229665>
I do not think you need this comment. You may see the same comment in this file, but that is because we will try the `offerId` as an offer first, and then try it as an inverse offer. I do think that is the case in this method.
src/master/validation.cpp (line 1337)
<https://reviews.apache.org/r/54650/#comment229664>
I would suggest to change to:
```
return Error("Offer " + stringify(offerId) + " is no longer valid");
```
src/master/validation.cpp (lines 1424 - 1431)
<https://reviews.apache.org/r/54650/#comment229668>
Suggest to change to:
```
if (role.isNone()) {
role = _role.get();
}
if (_role.get() != role.get()) {
return Error("...");
}
```
Similar with what we did in `validateSlave()`.
src/master/validation.cpp (lines 1425 - 1426)
<https://reviews.apache.org/r/54650/#comment229667>
Suggest to change to:
```
return Error("Offers for different roles in offer list);
```
Similar with the error message in `validateUniqueOfferID()`.
- Qian Zhang
On Dec. 12, 2016, 4:42 p.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54650/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2016, 4:42 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
>
>
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
>
>
> Repository: mesos
>
>
> Description
> -------
>
> For a multi-role framework, it may receive offers for different roles
> in that framework. However, we disallow multiple offers with different
> roles being accepted in single ACCEPT call.
>
>
> Diffs
> -----
>
> src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f
>
> Diff: https://reviews.apache.org/r/54650/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>