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...@cn.ibm.com> on 2016/12/02 08:44:31 UTC
Re: Review Request 54062: Introduced validateRoles method for
FrameworkInfo.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54062/
-----------------------------------------------------------
(Updated Dec. 2, 2016, 8:44 a.m.)
Review request for mesos, Benjamin Mahler, Guangya Liu, and Qiang Zhang.
Changes
-------
rebase and addresses comments from ben and guangya
Summary (updated)
-----------------
Introduced validateRoles method for FrameworkInfo.
Bugs: MESOS-6629
https://issues.apache.org/jira/browse/MESOS-6629
Repository: mesos
Description (updated)
-------
Roles should not contain duplicate entries. Also, we need to validate
that 'role', 'roles' and MULTI_ROLE capability are set properly. They
are checked against following matrix:
-- MULTI_ROLE is NOT set --
+-------+-------+---------+
| |Roles |No Roles |
+-------+-------+---------+
|Role | Error | None |
+-------+-------+---------+
|No Role| Error | None |
+-------+-------+---------+
---- MULTI_ROLE is set ----
+-------+-------+---------+
| |Roles |No Roles |
+-------+-------+---------+
|Role | Error | Error |
+-------+-------+---------+
|No Role| None | None |
+-------+-------+---------+
Diffs (updated)
-----
src/master/validation.hpp b8389460f34b3531f2b6ff93f18f496c01e1a079
src/master/validation.cpp 42d9b4a8784c2a161b74d7b46619cc22272e14e3
Diff: https://reviews.apache.org/r/54062/diff/
Testing
-------
make check
Thanks,
Jay Guo
Re: Review Request 54062: Introduced validateRoles method for
FrameworkInfo.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54062/#review157741
-----------------------------------------------------------
Ship it!
Ship It!
- Guangya Liu
On \u5341\u4e8c\u6708 2, 2016, 8:44 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54062/
> -----------------------------------------------------------
>
> (Updated \u5341\u4e8c\u6708 2, 2016, 8:44 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, Guangya Liu, and Qiang Zhang.
>
>
> Bugs: MESOS-6629
> https://issues.apache.org/jira/browse/MESOS-6629
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Roles should not contain duplicate entries. Also, we need to validate
> that 'role', 'roles' and MULTI_ROLE capability are set properly. They
> are checked against following matrix:
>
> -- MULTI_ROLE is NOT set --
> +-------+-------+---------+
> | |Roles |No Roles |
> +-------+-------+---------+
> |Role | Error | None |
> +-------+-------+---------+
> |No Role| Error | None |
> +-------+-------+---------+
>
> ---- MULTI_ROLE is set ----
> +-------+-------+---------+
> | |Roles |No Roles |
> +-------+-------+---------+
> |Role | Error | Error |
> +-------+-------+---------+
> |No Role| None | None |
> +-------+-------+---------+
>
>
> Diffs
> -----
>
> src/master/validation.hpp b8389460f34b3531f2b6ff93f18f496c01e1a079
> src/master/validation.cpp 42d9b4a8784c2a161b74d7b46619cc22272e14e3
>
> Diff: https://reviews.apache.org/r/54062/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>
Re: Review Request 54062: Introduced validateRoles method for
FrameworkInfo.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54062/#review157830
-----------------------------------------------------------
Ship it!
src/master/validation.cpp (lines 267 - 268)
<https://reviews.apache.org/r/54062/#comment228431>
Maybe we should put this in an else to be a bit more clear:
```
} else {
roles.insert(role);
}
```
I'll make this adjustment.
src/master/validation.cpp (lines 280 - 295)
<https://reviews.apache.org/r/54062/#comment228435>
Just as an FYI our error message pattern is to omit caller-available information when writing the error message in the callee. We happen to not follow this pattern in `roles::validate` which means that `roles::validate` will include the role in its error message, so we don't have to include it here.
Normally, our pattern would be to omit the role in the error from `roles::validate` and rather just say what's wrong with the role, so that the caller here would include the role:
```
return Error("'FrameworkInfo.roles' contains invalid role
" '" + role + "': " + error->message);
```
But, here because `roles::validate` is including the role in its message, this would double log the role:
```
'FrameworkInfo.roles' contains invalid role 'hello world': Role name 'hello world' is invalid because it contains slash, backslash or whitespace
```
src/master/validation.cpp (line 302)
<https://reviews.apache.org/r/54062/#comment228430>
Let's move this up to the declaration in the header, so that callers are aware of it. I'll make this adjustment.
- Benjamin Mahler
On Dec. 2, 2016, 8:44 a.m., Jay Guo wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54062/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2016, 8:44 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, Guangya Liu, and Qiang Zhang.
>
>
> Bugs: MESOS-6629
> https://issues.apache.org/jira/browse/MESOS-6629
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Roles should not contain duplicate entries. Also, we need to validate
> that 'role', 'roles' and MULTI_ROLE capability are set properly. They
> are checked against following matrix:
>
> -- MULTI_ROLE is NOT set --
> +-------+-------+---------+
> | |Roles |No Roles |
> +-------+-------+---------+
> |Role | Error | None |
> +-------+-------+---------+
> |No Role| Error | None |
> +-------+-------+---------+
>
> ---- MULTI_ROLE is set ----
> +-------+-------+---------+
> | |Roles |No Roles |
> +-------+-------+---------+
> |Role | Error | Error |
> +-------+-------+---------+
> |No Role| None | None |
> +-------+-------+---------+
>
>
> Diffs
> -----
>
> src/master/validation.hpp b8389460f34b3531f2b6ff93f18f496c01e1a079
> src/master/validation.cpp 42d9b4a8784c2a161b74d7b46619cc22272e14e3
>
> Diff: https://reviews.apache.org/r/54062/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jay Guo
>
>