This is an automatically generated e-mail. To reply, visit:

Looks good. Just some minor changes and it'll be good to go.

src/tests/role_tests.cpp (line 47)

    Why is this an instance member method of RoleTest? Seems idempotent and 
stateless enough that it could be a static method, or even a freestanding 
helper function. If this is only called once, why is it even a function at all?
    Oh.. it's copied from `MasterQuotaTest::createRequestBody()`.
    Either find a way to actually merge the two implementations, or just inline 
this into the one test that uses it.

src/tests/role_tests.cpp (line 107)

    Is this necessary? The defaultAgentResourcesString is 
    Overriding that means that Mesos will auto-detect cpus/mem

src/tests/role_tests.cpp (line 120)

    s/by default/the default/

src/tests/role_tests.cpp (line 145)

    And expect that it doesn't already contain dynamicallyReserved?

src/tests/role_tests.cpp (line 160)

    Could you also 

src/tests/role_tests.cpp (line 162)

    Why 'volume1' if there's no other volume? 'volume' should be fine.

src/tests/role_tests.cpp (lines 165 - 166)

    Can you make these "volumeId1" and "/container/path" so it's clearer what 
they're supposed to be?

src/tests/role_tests.cpp (line 181)

    And expect that it doesn't contain `unreserved` or `dynamicallyReserved` 

src/tests/role_tests.cpp (line 216)


src/tests/role_tests.cpp (lines 251 - 259)

    Why bother to send back the ack? Or even send the status back at all? As 
soon as you have a MockExecutor that receives a launchTask, you've verified 
that "when using implicit roles, a static reservation can be used to launch a 

src/tests/role_tests.cpp (line 379)

    Seems we don't have any tests with weights yet. Congrats on writing the 

src/tests/role_tests.cpp (line 397)

    You could push this AWAIT down next to the AWAIT for fwkId2. No reason to 
block starting driver2 because you're waiting on driver1 to get its fwkId. 
Really you just want to make sure both fwks are registered before you hit 

src/tests/role_tests.cpp (lines 483 - 484)

    Can you clarify what "the expected information" is? Looks like this tests 
that `/roles` shows a role that has quota configured, even if that role has no 
frameworks registered, no reserved resources, and default weight.

src/tests/role_tests.cpp (line 495)

    (because there are no slaves registered with any resources)

- Adam B

On Dec. 17, 2015, 5:55 p.m., Neil Conway wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41225/
> -----------------------------------------------------------
> (Updated Dec. 17, 2015, 5:55 p.m.)
> Review request for mesos, Adam B, Alexander Rukletsov, and Yongqiao Wang.
> Repository: mesos
> Description
> -------
> Added test cases for implicit roles.
> Diffs
> -----
>   src/tests/role_tests.cpp PRE-CREATION 
> Diff: https://reviews.apache.org/r/41225/diff/
> Testing
> -------
> Thanks,
> Neil Conway

Reply via email to