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