mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 56376: Updated allocator test to support create multi role framework.
Date Sat, 11 Feb 2017 00:41:48 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message