mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 53096: Fix handling in shared count in total resources in the sorter.
Date Sun, 11 Dec 2016 16:23:50 GMT

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




src/master/allocator/mesos/hierarchical.cpp (lines 722 - 727)
<https://reviews.apache.org/r/53096/#comment229554>

    Can we have a private helper
    
    ```
    void HierarchicalAllocatorProcess::updateTotal(
        const SlaveID& slaveId,
        const Resources& total)
    {
    ...
    }
    ```
    
    and have `updateAllocation()` and `updateAvailable()` both call it? Obviously this reduces
code duplication but I think more importantly we are able to comment in a single place about
the connection between the role sorter and the quota role sorter's `total` and the "total
resource size of the cluster" which is our rationale for this patch.
    
    Something like:
    
    ```
    Currently `roleSorter` and `quotaRoleSorter`, being the root-level sorters, maintain all
of `slaves[slaveId].total` (or the `nonRevocable()` portion in the case of `quotaRoleSorter`)
in their own totals. Therefore we update them using the same `updatedTotal` as used to update
`slaves[slaveId].total`.
    ```



src/tests/persistent_volume_tests.cpp (lines 1281 - 1282)
<https://reviews.apache.org/r/53096/#comment229557>

    The reason for this test, when not being looked at in the context of MESOS-6444, is a
bit hard to understand. How about we add to the end of the comments:
    
    ```
    This is to catch any regression to MESOS-6444.
    ```



src/tests/persistent_volume_tests.cpp (lines 1327 - 1329)
<https://reviews.apache.org/r/53096/#comment229559>

    This doesn't help here and we should be fine without it.
    
    If the agent registers first, when the scheduler connects it's going to get an offer without
waiting for the next allocation cycle.
    
    If the scheduler registers first, when the agent later registers, the master will generate
an offer to the scheduler without waiting for the next allocation cycle either.



src/tests/persistent_volume_tests.cpp (line 1342)
<https://reviews.apache.org/r/53096/#comment229625>

    Single space.



src/tests/persistent_volume_tests.cpp (line 1351)
<https://reviews.apache.org/r/53096/#comment229610>

    s/long lived task/a long-lived task/



src/tests/persistent_volume_tests.cpp (line 1360)
<https://reviews.apache.org/r/53096/#comment229611>

    s/short lived task/a short-lived task/



src/tests/persistent_volume_tests.cpp (line 1367)
<https://reviews.apache.org/r/53096/#comment229560>

    Use "exit 0" as a "simplest dummy task" since it really doesn't matter what it does.



src/tests/persistent_volume_tests.cpp (lines 1382 - 1384)
<https://reviews.apache.org/r/53096/#comment229612>

    Indentation. Also fix other occurrences in this review.



src/tests/persistent_volume_tests.cpp (lines 1383 - 1384)
<https://reviews.apache.org/r/53096/#comment229613>

    There's no guarantee that TASK_FINISHED for task2 comes in after TASK_RUNNING for task1.
    
    We could use the technique employed in /r/54134/.



src/tests/persistent_volume_tests.cpp (line 1446)
<https://reviews.apache.org/r/53096/#comment229614>

    This is your long-lived task.



src/tests/persistent_volume_tests.cpp (lines 1447 - 1449)
<https://reviews.apache.org/r/53096/#comment229615>

    It takes too long to wait for the long-lived task. It being 10 secs may not be enough
for super slow VMs but may be too long for fast machines.
    
    Can we make the long-lived task "sleep 1000" so it's really long-lived and then kill it
explicitly here?



src/tests/persistent_volume_tests.cpp (line 1481)
<https://reviews.apache.org/r/53096/#comment229624>

    s/SharedVolume/SharedPersistentVolume/
    
    for a consistent naming pattern.



src/tests/persistent_volume_tests.cpp (line 1521)
<https://reviews.apache.org/r/53096/#comment229618>

    s/1st/the 1st/



src/tests/persistent_volume_tests.cpp (line 1529)
<https://reviews.apache.org/r/53096/#comment229617>

    Single space.



src/tests/persistent_volume_tests.cpp (line 1552)
<https://reviews.apache.org/r/53096/#comment229620>

    "until" here reads a bit weird: we advance the clock once to trigger the allocator action;
we are not keeping advancing the clock "until" the allocator action is triggered.
    
    Maybe just:
    
    ```
    // Advance the clock to generate an offer from the recovered resources.
    ```



src/tests/persistent_volume_tests.cpp (line 1563)
<https://reviews.apache.org/r/53096/#comment229619>

    s/2nd/the 2nd/



src/tests/persistent_volume_tests.cpp (line 1571)
<https://reviews.apache.org/r/53096/#comment229623>

    Single space.



src/tests/persistent_volume_tests.cpp (line 1589)
<https://reviews.apache.org/r/53096/#comment229622>

    Ditto.



src/tests/persistent_volume_tests.cpp (lines 1594 - 1595)
<https://reviews.apache.org/r/53096/#comment229621>

    Move it down to the last line?


- Jiang Yan Xu


On Dec. 2, 2016, 1:43 p.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53096/
> -----------------------------------------------------------
> 
> (Updated Dec. 2, 2016, 1:43 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6444
>     https://issues.apache.org/jira/browse/MESOS-6444
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We maintain a single copy of shared resource in the role and quota
> sorter's total resources. So, when we update these resources, we
> remove the previous resources at this agent and add the new resources
> at this agent.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.cpp 3b759494071c4cae4b8b7dbcb0028df4146fc30e

>   src/tests/persistent_volume_tests.cpp b7c313781f3dd9ab4fd68ce23ccc044d50b734d8 
> 
> Diff: https://reviews.apache.org/r/53096/diff/
> 
> 
> Testing
> -------
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>


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