mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Adam B" <a...@mesosphere.io>
Subject Re: Review Request 35711: Disallow special characters in role name.
Date Wed, 24 Jun 2015 09:30:27 GMT

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


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