You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Zhitao Li <zh...@gmail.com> on 2016/12/27 11:23:58 UTC

Re: Review Request 53679: Implement capacity heuristic check related tests.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53679/
-----------------------------------------------------------

(Updated Dec. 27, 2016, 11:23 a.m.)


Review request for mesos, Alexander Rukletsov, Xiaojian Huang, and Kunal Thakar.


Bugs: MESOS-6840
    https://issues.apache.org/jira/browse/MESOS-6840


Repository: mesos


Description
-------

Implement capacity heuristic check related tests.


Diffs
-----

  src/tests/master_quota_tests.cpp 48be7406181646c8cc1d169b82a4a4ca71cdf03b 

Diff: https://reviews.apache.org/r/53679/diff/


Testing
-------


Thanks,

Zhitao Li

Re: Review Request 53679: Implement capacity heuristic check related tests.

Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53679/#review160148
-----------------------------------------------------------




src/tests/master_quota_tests.cpp (lines 1084 - 1087)
<https://reviews.apache.org/r/53679/#comment231214>

    We can fit this into three lines with some rephrasing:
    ```
    // Checks that a quota request is still satisfied even if tasks from other
    // roles use up the cluster capacity, because we do not consider tasks running
    // on unreserved resources when performing the capacity heuristic.
    ```



src/tests/master_quota_tests.cpp (line 1101)
<https://reviews.apache.org/r/53679/#comment231215>

    Ups?



src/tests/master_quota_tests.cpp (line 1107)
<https://reviews.apache.org/r/53679/#comment231185>

    Since there is a single agent in the test, enumerating it is misleading.
    
    Also please s/slave/agent everywhere.



src/tests/master_quota_tests.cpp (lines 1120 - 1124)
<https://reviews.apache.org/r/53679/#comment231216>

    Do you use `clang-format`? It suggests
    ```
    MesosSchedulerDriver driver(
        &sched, frameworkInfo, master.get()->pid, DEFAULT_CREDENTIAL);
    ```



src/tests/master_quota_tests.cpp (line 1138)
<https://reviews.apache.org/r/53679/#comment231184>

    Remove extra blank line. You may want to say that you would like to emulate a long running task consuming all available resources on the agent.



src/tests/master_quota_tests.cpp (lines 1156 - 1158)
<https://reviews.apache.org/r/53679/#comment231187>

    Let's move this inside the explicit `{}` block below.



src/tests/master_quota_tests.cpp (lines 1160 - 1162)
<https://reviews.apache.org/r/53679/#comment231186>

    How about this:
    ```
      // Submit a quota request asking for all "cpus" and "mem" resources from the only agent. Despite these resources are already allocated, the request should be satisfied because resources used by a non-quota'ed role are not accounted in the capacity heuristic check.
    ```



src/tests/master_quota_tests.cpp (lines 1182 - 1184)
<https://reviews.apache.org/r/53679/#comment231217>

    Let's rephrase a bit:
    ```
    // Checks that a quota request is satisfied even if some part of resources needed to fulfil the request is dynamically reserved for other role.
    ```



src/tests/master_quota_tests.cpp (lines 1194 - 1202)
<https://reviews.apache.org/r/53679/#comment231189>

    1. If you enumerate entities, you should do it consistently to avoid confusion.
    2. Let's be consistent in naming part of the same entity and go for "agent*" everywhere.
    3. Let's avoid using "slave" wherever possible. Please replace all "*slave*" occurences with "*agent*" here and everywhere.
    4. Feel free to file a separate patch to `s/slave/agent/` everywhere in this file to stay consistent.



src/tests/master_quota_tests.cpp (line 1207)
<https://reviews.apache.org/r/53679/#comment231218>

    Kill this line.



src/tests/master_quota_tests.cpp (line 1222)
<https://reviews.apache.org/r/53679/#comment231190>

    AFAIK, we don't call it "operation" when we hit the master endpoint.
    
    Also, please backtick types, functions, and variables in comments. Also, keep capitalization consistent, e.g., `role1` -> `ROLE`.



src/tests/master_quota_tests.cpp (lines 1222 - 1247)
<https://reviews.apache.org/r/53679/#comment231227>

    I was thinking about how we can make this snippet more readable and self-explaining. How about
    ```
    Future<CheckpointResourcesMessage> checkpointResources =
        FUTURE_PROTOBUF(CheckpointResourcesMessage(), _, slave1.get()->pid);
    
      // Dynamically reserve all "cpus" resources on `agent1` for `ROLE1`.
      auto reserveResourcesRequestBody = [=](const Resources& resources) -> string {
        Resources unreserved = resources.flatten(
            ROLE1,
            createReservationInfo(DEFAULT_CREDENTIAL.principal())).get();
    
        const google::protobuf::RepeatedPtrField<Resource>& repeated = unreserved;
    
        string reservationBody = strings::format(
            "slaveId=%s&resources=%s",
            slaveId->value(),
            JSON::protobuf(repeated)).get();
    
        return reservationBody;
      };
    
      Future<Response> reservationResponse = process::http::post(
          master.get()->pid,
          "reserve",
          createBasicAuthHeaders(DEFAULT_CREDENTIAL),
          reserveResourcesRequestBody(agent1TotalResources->get("cpus")));
    ```



src/tests/master_quota_tests.cpp (lines 1223 - 1225)
<https://reviews.apache.org/r/53679/#comment231191>

    Why not using `Resources::get()`?



src/tests/master_quota_tests.cpp (line 1252)
<https://reviews.apache.org/r/53679/#comment231238>

    Backtick all names in comments, here and everywhere.



src/tests/master_quota_tests.cpp (lines 1256 - 1266)
<https://reviews.apache.org/r/53679/#comment231192>

    Let's move this into the explicit scope below and consolidate comments before the scope.



src/tests/master_quota_tests.cpp (lines 1257 - 1263)
<https://reviews.apache.org/r/53679/#comment231193>

    Why not using `Resource::get()`?



src/tests/master_quota_tests.cpp (lines 1268 - 1269)
<https://reviews.apache.org/r/53679/#comment231194>

    How about this:
    ```
      // Submit a quota request asking for all "cpus" and "mem" resources in the cluster. Despite these resources are partially dynamically reserved, the request should be satisfied because dynamic reservations for a non-quota'ed role are not accounted in the capacity heuristic check.
    ```



src/tests/master_quota_tests.cpp (lines 1283 - 1284)
<https://reviews.apache.org/r/53679/#comment231239>

    // Checks that a quota request is not satisfied if some part of resources needed to fulfil the request is statically reserved for other role.



src/tests/master_quota_tests.cpp (line 1309)
<https://reviews.apache.org/r/53679/#comment231195>

    Let's explicitly use `ROLE1` constant.



src/tests/master_quota_tests.cpp (lines 1325 - 1326)
<https://reviews.apache.org/r/53679/#comment231196>

    Let's rephrase this.
    ```
      // Total cluster resources with stripped static reservation info.
    ```



src/tests/master_quota_tests.cpp (lines 1327 - 1331)
<https://reviews.apache.org/r/53679/#comment231197>

    We should probably use `Resoruces::flatten()` here to precisely express what we mean.



src/tests/master_quota_tests.cpp (lines 1333 - 1335)
<https://reviews.apache.org/r/53679/#comment231198>

    How about this:
    ```
      // Submit a quota request asking for all "cpus" and "mem" resources in the cluster regardless of their static allocation. Such request should not be satisfied because statically reserved resources are excluded from the pool of available resources in the capacity heuristic check.
    ```



src/tests/master_quota_tests.cpp (line 1340)
<https://reviews.apache.org/r/53679/#comment231199>

    You probably want to do it for the role other than `ROLE1`.



src/tests/master_quota_tests.cpp (line 1347)
<https://reviews.apache.org/r/53679/#comment231200>

    "... previous request..."



src/tests/master_quota_tests.cpp (line 1353)
<https://reviews.apache.org/r/53679/#comment231201>

    s/ROLE1/ROLE2



src/tests/master_quota_tests.cpp (lines 1361 - 1362)
<https://reviews.apache.org/r/53679/#comment231240>

    Keeping in mind you will merge the last test in here:
    ```
    // Checks that resources laid away to satisfy all accepted quotas are not used to fulfil the incoming quota request.
    ```
    
    Don't forget to rename the test as well!



src/tests/master_quota_tests.cpp (line 1368)
<https://reviews.apache.org/r/53679/#comment231202>

    Ups



src/tests/master_quota_tests.cpp (lines 1385 - 1390)
<https://reviews.apache.org/r/53679/#comment231242>

    This will change a bit when you merge `InsufficientResourcesMultipleQuotas` in



src/tests/master_quota_tests.cpp (lines 1427 - 1428)
<https://reviews.apache.org/r/53679/#comment231211>

    How about
    ```
    // Checks that resources from a disconnected agent are not used to fulfil quota requests.
    ```



src/tests/master_quota_tests.cpp (line 1433)
<https://reviews.apache.org/r/53679/#comment231212>

    Remove this extra line please.



src/tests/master_quota_tests.cpp (line 1451)
<https://reviews.apache.org/r/53679/#comment231243>

    Feel free to kill this empty line



src/tests/master_quota_tests.cpp (lines 1455 - 1458)
<https://reviews.apache.org/r/53679/#comment231244>

    Using `Resources::get()` is probably more readable. If you want to go with `.filter()`, update `AvailableResourcesWithTasks` for consistency.



src/tests/master_quota_tests.cpp (line 1473)
<https://reviews.apache.org/r/53679/#comment231245>

    s/resource/resources
    ... deactivates the only agent,...



src/tests/master_quota_tests.cpp (line 1487)
<https://reviews.apache.org/r/53679/#comment231246>

    ... previous request...



src/tests/master_quota_tests.cpp (lines 1501 - 1503)
<https://reviews.apache.org/r/53679/#comment231210>

    I think this test can be combined with `InsufficientResourcesWithStaticReservation`


- Alexander Rukletsov


On Dec. 27, 2016, 11:23 a.m., Zhitao Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53679/
> -----------------------------------------------------------
> 
> (Updated Dec. 27, 2016, 11:23 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Xiaojian Huang, and Kunal Thakar.
> 
> 
> Bugs: MESOS-6840
>     https://issues.apache.org/jira/browse/MESOS-6840
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implement capacity heuristic check related tests.
> 
> 
> Diffs
> -----
> 
>   src/tests/master_quota_tests.cpp 48be7406181646c8cc1d169b82a4a4ca71cdf03b 
> 
> Diff: https://reviews.apache.org/r/53679/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 53679: Implemented capacity heuristic check related tests.

Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53679/
-----------------------------------------------------------

(Updated Feb. 13, 2017, 7:09 p.m.)


Review request for mesos, Alexander Rukletsov, Xiaojian Huang, Kunal Thakar, and Neil Conway.


Bugs: MESOS-6840
    https://issues.apache.org/jira/browse/MESOS-6840


Repository: mesos


Description
-------

Implemented capacity heuristic check related tests.


Diffs (updated)
-----

  src/tests/master_quota_tests.cpp d15c7aacb85596cdee7cf59c0c179247ba624fe5 

Diff: https://reviews.apache.org/r/53679/diff/


Testing
-------


Thanks,

Zhitao Li

Re: Review Request 53679: Implemented capacity heuristic check related tests.

Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53679/#review165348
-----------------------------------------------------------




src/tests/master_quota_tests.cpp (line 1181)
<https://reviews.apache.org/r/53679/#comment237187>

    This line is too long and currently this chain does not apply cleanly. Please run commits through `support/mesos-style.py`.


- Benjamin Bannier


On Feb. 10, 2017, 8:55 a.m., Zhitao Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53679/
> -----------------------------------------------------------
> 
> (Updated Feb. 10, 2017, 8:55 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Xiaojian Huang, Kunal Thakar, and Neil Conway.
> 
> 
> Bugs: MESOS-6840
>     https://issues.apache.org/jira/browse/MESOS-6840
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented capacity heuristic check related tests.
> 
> 
> Diffs
> -----
> 
>   src/tests/master_quota_tests.cpp d15c7aacb85596cdee7cf59c0c179247ba624fe5 
> 
> Diff: https://reviews.apache.org/r/53679/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 53679: Implemented capacity heuristic check related tests.

Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53679/
-----------------------------------------------------------

(Updated Feb. 10, 2017, 7:55 a.m.)


Review request for mesos, Alexander Rukletsov, Xiaojian Huang, Kunal Thakar, and Neil Conway.


Changes
-------

Rebase with recent change.


Bugs: MESOS-6840
    https://issues.apache.org/jira/browse/MESOS-6840


Repository: mesos


Description
-------

Implemented capacity heuristic check related tests.


Diffs (updated)
-----

  src/tests/master_quota_tests.cpp d15c7aacb85596cdee7cf59c0c179247ba624fe5 

Diff: https://reviews.apache.org/r/53679/diff/


Testing
-------


Thanks,

Zhitao Li

Re: Review Request 53679: Implemented capacity heuristic check related tests.

Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53679/
-----------------------------------------------------------

(Updated Feb. 9, 2017, 9:42 p.m.)


Review request for mesos, Alexander Rukletsov, Xiaojian Huang, and Kunal Thakar.


Changes
-------

Review comments.


Bugs: MESOS-6840
    https://issues.apache.org/jira/browse/MESOS-6840


Repository: mesos


Description
-------

Implemented capacity heuristic check related tests.


Diffs (updated)
-----

  src/tests/master_quota_tests.cpp b25f5911579c435549b9bc65994627414357dcb6 

Diff: https://reviews.apache.org/r/53679/diff/


Testing
-------


Thanks,

Zhitao Li

Re: Review Request 53679: Implemented capacity heuristic check related tests.

Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53679/#review164651
-----------------------------------------------------------




src/tests/master_quota_tests.cpp (line 1134)
<https://reviews.apache.org/r/53679/#comment236415>

    `EXPECT_FALSE(offers->empty());`



src/tests/master_quota_tests.cpp (line 1136)
<https://reviews.apache.org/r/53679/#comment236416>

    Let's move this just before the invocation of `driver->launchTasks`.



src/tests/master_quota_tests.cpp (line 1150)
<https://reviews.apache.org/r/53679/#comment236417>

    `status->state()`



src/tests/master_quota_tests.cpp (lines 1158 - 1159)
<https://reviews.apache.org/r/53679/#comment236419>

    Any reason we need to reference `defaultAgentResources` here at all? I see you make sure above that `defaultAgentResources == agent1TotalResources`, but we could work with less symbols in the scope if we'd just use `agent1TotalResources`.



src/tests/master_quota_tests.cpp (line 1165)
<https://reviews.apache.org/r/53679/#comment236420>

    No need for the explicit `false` here since it is the default. I also find it not very readible (_what is `false`?_).



src/tests/master_quota_tests.cpp (line 1168)
<https://reviews.apache.org/r/53679/#comment236418>

    `response->body`



src/tests/master_quota_tests.cpp (line 1172)
<https://reviews.apache.org/r/53679/#comment236414>

    I am surprised this would not be called deterministically. Is that the intended behavior or a bug?



src/tests/master_quota_tests.cpp (line 1201)
<https://reviews.apache.org/r/53679/#comment236421>

    Let's use `agent1Id` or similar instead to not confuse this with the id of `agent2`'s possible ID.



src/tests/master_quota_tests.cpp (line 1203)
<https://reviews.apache.org/r/53679/#comment236422>

    We don't make use of this anywhere in the test.



src/tests/master_quota_tests.cpp (line 1215)
<https://reviews.apache.org/r/53679/#comment236423>

    We don't make use of this anywhere in the test.



src/tests/master_quota_tests.cpp (line 1221)
<https://reviews.apache.org/r/53679/#comment236425>

    Can you use explicit capture here?



src/tests/master_quota_tests.cpp (line 1227)
<https://reviews.apache.org/r/53679/#comment236430>

    This implicit conversion does not return a reference, but a new value instead; we do not use const ref to extend lifetime of temporaries.
    
    Instead just make the copy explicit by dropping the ref.



src/tests/master_quota_tests.cpp (line 1252)
<https://reviews.apache.org/r/53679/#comment236431>

    Unneeded capture.



src/tests/master_quota_tests.cpp (line 1255)
<https://reviews.apache.org/r/53679/#comment236432>

    Unneeded capture.



src/tests/master_quota_tests.cpp (line 1298)
<https://reviews.apache.org/r/53679/#comment236433>

    We don't rely on this anywhere in the test.



src/tests/master_quota_tests.cpp (line 1318)
<https://reviews.apache.org/r/53679/#comment236437>

    Let's not put expressions which can fail in the expected part here.
    
    How about
    
        EXPECT_EQ(agent2Resources, stringify(agent2TotalResources.get());
        
    Note that this requires inserting some spaces into `agent2Resources`, e.g., `cpus(%s):1; mem:512` etc.



src/tests/master_quota_tests.cpp (line 1325)
<https://reviews.apache.org/r/53679/#comment236413>

    Extra whitespace at EOL.



src/tests/master_quota_tests.cpp (line 1328)
<https://reviews.apache.org/r/53679/#comment236438>

    `// Satisfied`



src/tests/master_quota_tests.cpp (line 1337)
<https://reviews.apache.org/r/53679/#comment236439>

    Let's add an expectation for `response->body`.



src/tests/master_quota_tests.cpp (line 1377)
<https://reviews.apache.org/r/53679/#comment236440>

    We could use `agentTotalResources` and avoid pulling more variables into this scope.



src/tests/master_quota_tests.cpp (line 1422)
<https://reviews.apache.org/r/53679/#comment236442>

    Let's add an expectation for `response->body`.



src/tests/master_quota_tests.cpp (line 1465)
<https://reviews.apache.org/r/53679/#comment236443>

    We could just use `agent1TotalResources` in this test, and avoid pulling another variable into scope.



src/tests/master_quota_tests.cpp (line 1478)
<https://reviews.apache.org/r/53679/#comment236444>

    `slaveRegisteredMessage->to`



src/tests/master_quota_tests.cpp (line 1492)
<https://reviews.apache.org/r/53679/#comment236445>

    Let's add an expectation for `response->body`.


- Benjamin Bannier


On Feb. 7, 2017, 8:11 a.m., Zhitao Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53679/
> -----------------------------------------------------------
> 
> (Updated Feb. 7, 2017, 8:11 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Xiaojian Huang, and Kunal Thakar.
> 
> 
> Bugs: MESOS-6840
>     https://issues.apache.org/jira/browse/MESOS-6840
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented capacity heuristic check related tests.
> 
> 
> Diffs
> -----
> 
>   src/tests/master_quota_tests.cpp b25f5911579c435549b9bc65994627414357dcb6 
> 
> Diff: https://reviews.apache.org/r/53679/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 53679: Implemented capacity heuristic check related tests.

Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53679/
-----------------------------------------------------------

(Updated Feb. 7, 2017, 7:11 a.m.)


Review request for mesos, Alexander Rukletsov, Xiaojian Huang, and Kunal Thakar.


Changes
-------

Rebase and review comments.


Summary (updated)
-----------------

Implemented capacity heuristic check related tests.


Bugs: MESOS-6840
    https://issues.apache.org/jira/browse/MESOS-6840


Repository: mesos


Description (updated)
-------

Implemented capacity heuristic check related tests.


Diffs (updated)
-----

  src/tests/master_quota_tests.cpp b25f5911579c435549b9bc65994627414357dcb6 

Diff: https://reviews.apache.org/r/53679/diff/


Testing
-------


Thanks,

Zhitao Li