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 53479: Perform agent GC asynchronously.
Date Fri, 14 Jul 2017 08:47:33 GMT

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




src/slave/gc.hpp
Lines 122-123 (patched)
<https://reviews.apache.org/r/53479/#comment255728>

    No need for pointer?
    
    Just `process::Promise<Nothing> promise;` which is default initialized.



src/slave/gc.cpp
Line 93 (original), 103 (patched)
<https://reviews.apache.org/r/53479/#comment255731>

    const &



src/slave/gc.cpp
Lines 150-152 (patched)
<https://reviews.apache.org/r/53479/#comment255732>

    This comment doesn't seem necessary, we may have missed this initially but retrospectively
resetting `removing` before starting async rmdir feels straighforward.



src/slave/gc.cpp
Lines 153 (patched)
<https://reviews.apache.org/r/53479/#comment255730>

    const &



src/slave/gc.cpp
Lines 155 (patched)
<https://reviews.apache.org/r/53479/#comment255734>

    Also add the action taken here, which is 
    
    ```
    VLOG(1) << "Skipping deletion of '" << info-> path << "'  as it is
already in progress";
    ```



src/slave/gc.cpp
Lines 159 (patched)
<https://reviews.apache.org/r/53479/#comment255735>

    Not yours but quoting the path makes logging more consistent.
    
    Also move this into the lambda above each rmdir because this log does suggest the immediate
rmdir for this path?



src/slave/gc.cpp
Lines 163 (patched)
<https://reviews.apache.org/r/53479/#comment255733>

    I think the name and type `bool removing` is self-evident.



src/slave/gc.cpp
Lines 205 (patched)
<https://reviews.apache.org/r/53479/#comment255737>

    `CHECK_READY(result);`?



src/slave/gc.cpp
Lines 206 (patched)
<https://reviews.apache.org/r/53479/#comment255740>

    Kill this since we have the CHECK_READY.



src/slave/gc.cpp
Lines 207 (patched)
<https://reviews.apache.org/r/53479/#comment255736>

    const &



src/slave/gc.cpp
Lines 210 (patched)
<https://reviews.apache.org/r/53479/#comment255738>

    Use `CHECK_EQ(timeouts.erase(info->path), 1);`?


- Jiang Yan Xu


On July 13, 2017, 2:48 p.m., Jacob Janco wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53479/
> -----------------------------------------------------------
> 
> (Updated July 13, 2017, 2:48 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6549
>     https://issues.apache.org/jira/browse/MESOS-6549
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> - Previously, rmdir operations were serialized
>   through the garbage collector. Expensive removal
>   events had the potential to delay task launch.
> - MESOS-6549
> 
> 
> Diffs
> -----
> 
>   src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e 
>   src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58 
> 
> 
> Diff: https://reviews.apache.org/r/53479/diff/6/
> 
> 
> Testing
> -------
> 
> `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>


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