----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54062/#review156920 -----------------------------------------------------------
src/master/validation.hpp (lines 60 - 61) <https://reviews.apache.org/r/54062/#comment227287> new line here as `//` src/master/validation.hpp (line 61) <https://reviews.apache.org/r/54062/#comment227283> ``` --- MULTI_ROLE is NOT set --- ``` src/master/validation.hpp (line 69) <https://reviews.apache.org/r/54062/#comment227284> Add `//` here src/master/validation.hpp (line 70) <https://reviews.apache.org/r/54062/#comment227285> ``` --- MULTI_ROLE is set --- ``` src/master/validation.hpp (lines 77 - 78) <https://reviews.apache.org/r/54062/#comment227286> new line here as `//` src/master/validation.cpp (line 249) <https://reviews.apache.org/r/54062/#comment227288> s/FrameworkInfo.role/'FrameworkInfo.role' s/FrameworkInfo.roles/'FrameworkInfo.roles' Ditto for other error messages. src/master/validation.cpp (lines 264 - 265) <https://reviews.apache.org/r/54062/#comment227289> How about ``` return Error("The MULTI_ROLE framework capability must be enabled" " when 'FrameworkInfo.roles' is set"); ``` src/master/validation.cpp (lines 295 - 305) <https://reviews.apache.org/r/54062/#comment227290> How about the following? ``` // TODO(jay_guo) Consider extending this method to validate more // fields of FrameworkInfo in the future. Option<Error> validate(const mesos::FrameworkInfo& frameworkInfo) { vector<lambda::function<Option<Error>()>> validators = { lambda::bind(internal::validateRoles, frameworkInfo) }; foreach (const lambda::function<Option<Error>()>& validator, validators) { Option<Error> error = validator(); if (error.isSome()) { return error; } } return None(); } ``` src/master/validation.cpp (line 301) <https://reviews.apache.org/r/54062/#comment227291> I think we do not need to mention `Invalid roles` here as here we are checking both `role` and `roles`, using the error returned by `internal::validateRoles` is good enough. src/tests/master_validation_tests.cpp (lines 2333 - 2335) <https://reviews.apache.org/r/54062/#comment227300> ``` // This tests the validate roles function of FrameworkInfo. For 3 // attributes including role, roles and MULTI_ROLE capability, // we have 2^3 cases to test in total. ``` src/tests/master_validation_tests.cpp (line 2390) <https://reviews.apache.org/r/54062/#comment227295> keep align with `FrameworkValidationTest` src/tests/master_validation_tests.cpp (line 2415) <https://reviews.apache.org/r/54062/#comment227296> ``` // This test ensures subscription succeeds for multiple role // framework when MULTI_ROLE capability is enabled. ``` src/tests/master_validation_tests.cpp (line 2423) <https://reviews.apache.org/r/54062/#comment227297> <p></p> <div class="codehilite"><pre>// Add multiple roles to the FrameworkInfo with MULTI_ROLE // capability is enabled. </pre></div> src/tests/master_validation_tests.cpp (line 2454) <https://reviews.apache.org/r/54062/#comment227298> ``` // Add multiple roles to the FrameworkInfo with MULTI_ROLE // capability is enabled. ``` - Guangya Liu On 十一月 25, 2016, 11:26 a.m., Jay Guo wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54062/ > ----------------------------------------------------------- > > (Updated 十一月 25, 2016, 11:26 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 > ------- > > We need to do necessary validation for the conflicts of role, roles > and MULTI_ROLE capability. It complies with 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 | > |-------|-------|---------| > > One test case is added to ensure validateRoles() catches invalid > combination of these attributes. Another three test cases are added > to ensure the master accepts/rejects subscription given valid/invalid > multiple roles. > > > Diffs > ----- > > src/master/master.cpp e03a2e8025943825a2902102c43dc0eb66bacb6a > src/master/validation.hpp b8389460f34b3531f2b6ff93f18f496c01e1a079 > src/master/validation.cpp 42d9b4a8784c2a161b74d7b46619cc22272e14e3 > src/tests/master_validation_tests.cpp > f893067859425967654401f3226149268b51cf57 > > Diff: https://reviews.apache.org/r/54062/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jay Guo > >
