> On Feb. 8, 2017, 2:48 p.m., Jay Guo wrote: > > src/tests/hierarchical_allocator_tests.cpp, lines 203-205 > > <https://reviews.apache.org/r/56376/diff/1/?file=1626287#file1626287line203> > > > > IMO, since this is for testing purpose, we probably don't need to > > rigidly require test writers to construct frameworkInfo with `MULTI_ROLE` > > capability. If `createFrameworkInfo` is called with multiple roles, we > > could simply add `MULTI_ROLE` implicitly. And one could still explicitly > > add `MULTI_ROLE` to capabilities with a single role. Then we don't need to > > change existing test cases and avoid future confusion. What do you think? > > Guangya Liu wrote: > Yes, but the only problem is that we cannot specfify multiple roles with > the first paramter here `const string& role`, so I have to update it to > `const set<string>& roles`. > > Jay Guo wrote: > I agree that tests need to be updated to pass in `set<string>`. However, > I feel return type `Try<FrameworkInfo>` is a bit complicated. In this form, > should we perform `isSome()` check? I would suggest to stick to > `FrameworkInfo createFrameworkInfo(...)`. > > Guangya Liu wrote: > As I added some error handling as folllowing, so I have to return `Try` > here, any comments? > > ``` > if (!multiRole && roles.size() > 1) { > return Error("Non multi-role framework support only one role"); > } > ```
As I mentioned previously, we could add `MULTI_ROLE` implicitly for this case. This method is for testing purpose and we could expect a test writer to be aware of the implications, instead of writing unecessary validation code in there test (to handle `Try<>`). - Jay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56376/#review164638 ----------------------------------------------------------- On Feb. 7, 2017, 6:10 p.m., Guangya Liu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56376/ > ----------------------------------------------------------- > > (Updated Feb. 7, 2017, 6:10 p.m.) > > > Review request for mesos, Benjamin Mahler and Jay Guo. > > > Bugs: MESOS-6638 > https://issues.apache.org/jira/browse/MESOS-6638 > > > Repository: mesos > > > Description > ------- > > Updated allocator test to support create multi role framework. > > > Diffs > ----- > > src/tests/hierarchical_allocator_tests.cpp > c681d03c3f94f7d071143366a5aad0421108ebec > > Diff: https://reviews.apache.org/r/56376/diff/ > > > Testing > ------- > > make > make check > > > Thanks, > > Guangya Liu > >
