mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rukletsov <ruklet...@gmail.com>
Subject Re: Review Request 53679: Implement capacity heuristic check related tests.
Date Tue, 27 Dec 2016 20:11:06 GMT

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


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