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


Looks great! We recently added validation lambdas to flag definitions, so you 
may be able to take advantage of that.
We probably also want to do validation in the slave for the --resources flag, 
since resources can be declared as reserved for particular roles, and reserving 
a resource for an invalid role would be bad. I don't know if we even validate 
the reservation roles as known roles in the master upon slave registration. The 
only difference with --resources is that resources can use `*` to declare 
resources as unreserved.


src/master/master.hpp (lines 54 - 55)
<https://reviews.apache.org/r/35711/#comment141738>

    Alphabetize, please



src/master/master.hpp (line 632)
<https://reviews.apache.org/r/35711/#comment141743>

    We should also disallow role name `*` in master --roles because it is used 
to indicate unreserved resources.



src/master/master.hpp (line 635)
<https://reviews.apache.org/r/35711/#comment141740>

    "Role name 'foobar' is invalid because it starts with a dash."



src/master/master.hpp (lines 638 - 639)
<https://reviews.apache.org/r/35711/#comment141741>

    Why not use string::find()?



src/master/master.hpp (line 641)
<https://reviews.apache.org/r/35711/#comment141742>

    How about "Role name 'foobar' is invalid because it contains xyz."? (here 
and below)



src/master/master.cpp (line 558)
<https://reviews.apache.org/r/35711/#comment141739>

    Consider using the new flags validation lambdas as added by 
https://reviews.apache.org/r/34943
    I know it will require you to tokenize/iterate through the roles twice, but 
that's probably not much of a performance hit.


- Adam B


On June 21, 2015, 7:21 a.m., haosdent huang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
> 
> (Updated June 21, 2015, 7:21 a.m.)
> 
> 
> Review request for mesos, Adam B and Jie Yu.
> 
> 
> Bugs: MESOS-2210
>     https://issues.apache.org/jira/browse/MESOS-2210
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Disallow special characters in role name.
> 
> 
> Diffs
> -----
> 
>   src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064 
>   src/master/master.cpp 0135c155181546d3cb43e9e05bb874af846d928d 
> 
> Diff: https://reviews.apache.org/r/35711/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Reply via email to