mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jay Guo <guojian...@cn.ibm.com>
Subject Re: Review Request 54062: Introduced validateRoles method for FrameworkInfo.
Date Fri, 02 Dec 2016 08:46:57 GMT


> On Dec. 1, 2016, 10:36 p.m., Benjamin Mahler wrote:
> > src/master/validation.cpp, lines 245-290
> > <https://reviews.apache.org/r/54062/diff/6/?file=1572831#file1572831line245>
> >
> >     We could pull out a `multiRole` boolean here and perform validation based on
it, this might make the MULTI_ROLE vs non-MULTI_ROLE distinction a bit clearer. I also noticed
that the first check (for both being set) isn't necessary?
> >     
> >     Here's a suggestion for this logic:
> >     
> >     ```
> >       bool multiRole = frameworkHasCapability(
> >           frameworkInfo,
> >           FrameworkInfo::Capability::MULTI_ROLE;
> >     
> >       // Ensure that the right fields are used.
> >       if (multiRole) {
> >         if (frameworkInfo.has_role()) {
> >           return Error("'FrameworkInfo.role' must not be set when the
> >                         framework is MULTI_ROLE capable");
> >         }
> >       } else {
> >         if (frameworkInfo.roles_size() > 0) {
> >           return Error("'FrameworkInfo.roles' must not be set when the
> >                         framework is not MULTI_ROLE capable");
> >         }
> >       }
> >     
> >       // Check for duplicate entries.
> >       //
> >       // TODO(bmahler): Use a generic duplicate check function.
> >       if (multiRole) {
> >         const hashset<string> duplicateRoles = [&]() {
> >           hashset<string> roles;
> >           hashset<string> duplicates;
> >         
> >           foreach (const string& role, frameworkInfo.roles()) {
> >             if (roles.contains(role)) {
> >               duplicates.insert(role);
> >             }
> >             roles.insert(role);
> >           }
> >         
> >           return duplicateRoles;
> >         }();
> >         
> >         if (!duplicateRoles.empty()) {
> >           return Error("'FrameworkInfo.roles' contains duplicate items: " +
> >                        stringify(duplicateRoles));
> >         }
> >       }
> >       
> >     
> >       // Validate the role(s).
> >       if (multiRole) {
> >         foreach (const string& role, frameworkInfo.roles()) {
> >           Option<Error> error = validate(role);
> >           if (error) {
> >             return Error("'FrameworkInfo.roles' contains invalid role(s)"
> >                          ": " + error->message);
> >           }
> >         }
> >       } else {
> >         Option<Error> error = validate(frameworkInfo.role());
> >         if (error) {
> >           return Error("'FrameworkInfo.role' is not a valid role"
> >                        ": " + error->message);
> >         }
> >       }
> >     
> >       return None();
> >     ```
> 
> Qian Zhang wrote:
>     I think one case is missed here: `MULTI_ROLE` is set but `FrameworkInfo.roles` is
empty, in this case, I think we should return an error since framework must have at least
one role in order to be offered resources.

This is a valid case and framework with this setting will not get any resource allocated.


- Jay


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54062/#review157651
-----------------------------------------------------------


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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message