> On Feb. 8, 2017, 6:48 a.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"); > } > ``` > > Jay Guo wrote: > 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<>`).
It seems to me we should just always inject the MULTI_ROLE capability since the allocator never looks at whether the framework is MULTI_ROLE capable. The logic between a non-MULTI_ROLE scheduler and a single role MULTI_ROLE scheduler is the same as far as the allocator is concerned. And let's document why we chose to do this. How does that sound? Just as an aside, you can use `FAIL() << "XXX"` if the caller makes a programming error that you want to guard against. Unlike CHECK, it doesn't crash the program, rather it just fails the currently running test. But if we were to always inject AllocationInfo - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56376/#review164638 ----------------------------------------------------------- On Feb. 7, 2017, 10:10 a.m., Guangya Liu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56376/ > ----------------------------------------------------------- > > (Updated Feb. 7, 2017, 10:10 a.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 > >
