Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review114439 --- Patch looks great! Reviews applied: [35711] Passed command:

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review114435 --- Ship it! Looks great! Just a few minor items that I can fix

Re: Review Request 35711: Disallow special characters in role name.

> On Jan. 14, 2016, 9:33 a.m., Adam B wrote: > > Looks great! Just a few minor items that I can fix myself. I'll commit this > > shortly. Thank you very much! - haosdent --- This is an automatically generated e-mail. To reply, visit:

Re: Review Request 35711: Disallow special characters in role name.

> On Jan. 14, 2016, 1:33 a.m., Adam B wrote: > > Looks great! Just a few minor items that I can fix myself. I'll commit this > > shortly. > > haosdent huang wrote: > Thank you very much! No sir, thank you! - Adam --- This is an

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated Jan. 14, 2016, 6 a.m.) Review request for mesos, Adam B, Jie Yu, and

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review114367 --- Ship it! Ship It! - Klaus Ma On Jan. 14, 2016, 1:12 a.m.,

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated Jan. 13, 2016, 5:12 p.m.) Review request for mesos, Adam B, Jie Yu,

Re: Review Request 35711: Disallow special characters in role name.

> On Jan. 10, 2016, 7:44 a.m., Klaus Ma wrote: > > @klaus1982 @adam-mesos Thank you very much for your detail reviews. Could you help review again? Thank you very much. - haosdent --- This is an automatically generated e-mail. To

Re: Review Request 35711: Disallow special characters in role name.

> On Jan. 10, 2016, 3:44 p.m., Klaus Ma wrote: > > src/common/roles.cpp, line 34 > > > > > > Do we need to check whether duplicated roles here? > > Adam B wrote: > Not necessary here, since they're de-duped

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review113644 --- Ship it! Looks good to me. We can commit this once you've

Re: Review Request 35711: Disallow special characters in role name.

> On Jan. 10, 2016, 7:44 a.m., Klaus Ma wrote: > > src/common/roles.cpp, lines 56-63 > > > > > > Just another question about the role: do we support other language, > > e.g. Chinese? If not, I'd suggest also to

Re: Review Request 35711: Disallow special characters in role name.

> On Jan. 7, 2016, 10:51 p.m., Adam B wrote: > > Still wondering if `class Roles` is any better than `namespace roles`, and > > unsure about the need for the CMake change. Thank you very much for your great reviews! Could you help review again? I change to use namespaces roles. - haosdent

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated Jan. 9, 2016, 11:15 a.m.) Review request for mesos, Adam B, Jie Yu,

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review113639 --- include/mesos/roles.hpp (line 51)

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review113287 --- Still wondering if `class Roles` is any better than `namespace

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review112788 --- Looks good. My biggest question is whether `class Roles` needs to

Re: Review Request 35711: Disallow special characters in role name.

> On Jan. 5, 2016, 8:16 a.m., Adam B wrote: > > Looks good. My biggest question is whether `class Roles` needs to exist, or > > whether we can just use freestanding `roles::validate()` functions. Thank you very much. I use vector instead. Could you help review again? - haosdent

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated Jan. 5, 2016, 3:19 p.m.) Review request for mesos, Adam B, Jie Yu,

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review112878 --- Patch looks great! Reviews applied: [35711] Passed command:

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated Jan. 5, 2016, 2:42 a.m.) Review request for mesos, Adam B, Jie Yu,

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review112771 --- include/mesos/roles.hpp (lines 29 - 49)

Re: Review Request 35711: Disallow special characters in role name.

> On 一月 5, 2016, 5:51 a.m., Guangya Liu wrote: > > include/mesos/roles.hpp, lines 29-49 > > > > > > It is better use /**/ for comments in a header file. > > haosdent huang wrote: > According resources.hpp,

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated Jan. 5, 2016, 6:58 a.m.) Review request for mesos, Adam B, Jie Yu,

Re: Review Request 35711: Disallow special characters in role name.

> On Jan. 5, 2016, 5:51 a.m., Guangya Liu wrote: > > include/mesos/roles.hpp, lines 29-49 > > > > > > It is better use /**/ for comments in a header file. > > haosdent huang wrote: > According resources.hpp,

Re: Review Request 35711: Disallow special characters in role name.

> On 一月 5, 2016, 5:51 a.m., Guangya Liu wrote: > > include/mesos/roles.hpp, lines 29-49 > > > > > > It is better use /**/ for comments in a header file. > > haosdent huang wrote: > According resources.hpp,

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review110461 --- Thanks for reviving this, and sorry it's taken so long to get back

Re: Review Request 35711: Disallow special characters in role name.

> On Dec. 15, 2015, 2:45 a.m., Adam B wrote: > > Thanks for reviving this, and sorry it's taken so long to get back to it. You'll also need to add some documentation about what the valid/invalid role names are, and when they'll be rejected. - Adam

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review110341 --- Looking good! Minor nit below. src/tests/roles_tests.cpp (line

Re: Review Request 35711: Disallow special characters in role name.

> On Nov. 5, 2015, 11:59 p.m., Neil Conway wrote: > > include/mesos/roles.hpp, line 51 > > > > > > Should this be a member function? i.e., not static. When I write this, I refer to the static validate method in

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated Dec. 12, 2015, 2:29 p.m.) Review request for mesos, Adam B, Jie Yu,

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated Dec. 12, 2015, 2:33 p.m.) Review request for mesos, Adam B, Jie Yu,

Re: Review Request 35711: Disallow special characters in role name.

> On Nov. 9, 2015, 11:26 p.m., Greg Mann wrote: > > src/master/master.cpp, line 568 > > > > > > Should we add a comment here saying that the validation of roles occurs > > in flags.cpp? I put off the check to

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review105756 --- include/mesos/roles.hpp (lines 40 - 41)

Re: Review Request 35711: Disallow special characters in role name.

> On Nov. 5, 2015, 3:59 p.m., Neil Conway wrote: > > Can we add an end-to-end unit test that verifies that attempting to > > register as a framework with an invalid role name results in an error? Attempting to register with an unrecognized role name will fail long before we get to creating a

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review105316 --- Can we add an end-to-end unit test that verifies that attempting

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review102158 --- src/Makefile.am (line 467)

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review101411 --- Patch looks great! Reviews applied: [35711] All tests passed. -

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated Oct. 3, 2015, 4:11 p.m.) Review request for mesos, Adam B, Jie Yu,

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review94631 --- Ship it! Ship It! - Guangya Liu On 八月 7, 2015, 6:50 p.m.,

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review94576 --- Patch looks great! Reviews applied: [35711] All tests passed. -

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated Aug. 7, 2015, 3:48 p.m.) Review request for mesos, Adam B, Jie Yu,

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated Aug. 7, 2015, 3:40 p.m.) Review request for mesos, Adam B, Jie Yu,

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated Aug. 7, 2015, 6:50 p.m.) Review request for mesos, Adam B, Jie Yu,

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review94571 --- Bad patch! Reviews applied: [35711] Failed command: make -j3

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated June 30, 2015, 8:29 a.m.) Review request for mesos, Adam B and Jie

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated June 30, 2015, 8:30 a.m.) Review request for mesos, Adam B and Jie

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated June 30, 2015, 4:56 p.m.) Review request for mesos, Adam B and Jie

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review89630 --- Looks great. I think you're covering all the places where roles are

Re: Review Request 35711: Disallow special characters in role name.

On June 29, 2015, 11:17 a.m., Adam B wrote: src/common/validation.hpp, lines 27-34 https://reviews.apache.org/r/35711/diff/7/?file=993941#file993941line27 I'm not convinced this is the right namespace/scope for a freestanding validate() function. Maybe, like we have a Resource

Re: Review Request 35711: Disallow special characters in role name.

On June 29, 2015, 11:17 a.m., Adam B wrote: src/common/validation.hpp, lines 27-34 https://reviews.apache.org/r/35711/diff/7/?file=993941#file993941line27 I'm not convinced this is the right namespace/scope for a freestanding validate() function. Maybe, like we have a Resource

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated June 27, 2015, 8:36 a.m.) Review request for mesos, Adam B and Jie

Re: Review Request 35711: Disallow special characters in role name.

On June 24, 2015, 9:30 a.m., Adam B wrote: 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

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated June 27, 2015, 11:32 a.m.) Review request for mesos, Adam B and Jie

Re: Review Request 35711: Disallow special characters in role name.

On June 24, 2015, 6:34 p.m., Jie Yu wrote: src/master/master.hpp, lines 630-651 https://reviews.apache.org/r/35711/diff/4/?file=989196#file989196line630 Can you move this function to src/master/validation.hpp|cpp ``` namespace role { OptionError

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated June 27, 2015, 11:35 a.m.) Review request for mesos, Adam B and Jie

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review89627 --- src/Makefile.am (line 1489)

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review89626 --- src/Makefile.am (line 362)

Re: Review Request 35711: Disallow special characters in role name.

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

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review89222 --- +1 on Adam's comments. src/master/master.hpp (lines 630 - 651)

Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- Review request for mesos, Adam B and Jie Yu. Repository: mesos Description

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated June 21, 2015, 2:15 p.m.) Review request for mesos, Adam B and Jie