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 Sun, 25 Jun 2017 07:33:16 GMT

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




src/slave/gc.hpp
Lines 126-131 (patched)
<https://reviews.apache.org/r/53479/#comment253124>

    There is still a bit of unclarity between the two, probably come the ambiguity of the
concepts gc vs. scheduled (pending, not in progress), rmdir (in progress), etc.
    
    Also the fact that when a path is removed we have to set two promises is a bit odd. 
    
    I commented on a possible simplication below and with that I think we can just leave the
field `promise` as is and add a `bool removing`:
    
    ```
    const process::Owned<process::Promise<Nothing>> promise;
    bool removing = false;
    ```
    
    Let's discuss it.



src/slave/gc.cpp
Line 63 (original), 71 (patched)
<https://reviews.apache.org/r/53479/#comment253117>

    This is the promise that sets `gc` in the changed `PathInfo` (as opposed to `removal`
there) right?
    
    If the callers should not be concerns about the distinction, perhaps just keep the name
`promise`.
    
    Anyhow, this comment might not be relevant if we use a `bool removing`.



src/slave/gc.cpp
Lines 106-107 (patched)
<https://reviews.apache.org/r/53479/#comment253118>

    The statement sounds like describing a race condition but rather returning `false` is
just because the gc is not prevented?
    
    Perhaps just consolidate with the comment about that we "return false (gc is not unscheduled)
after rmdir completes?"



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

    If we follow the suggestions below, here it could be 
    
    ```
    if (info.removing) {
      return info.promise->future()
        .then([]() { return false; });
    }
    ```
    
    ?



src/slave/gc.cpp
Line 153 (original), 149-178 (patched)
<https://reviews.apache.org/r/53479/#comment253129>

    Would this be simpler?
    
    ```
        list<PathInfo> infos = paths.get(removalTime);
    
        auto rmdirs = [infos]() {
          foreach (const PathInfo& info, infos) {
            LOG(INFO) << "Deleting " << info.path;
    
            // ...
            Try<Nothing> rmdir = os::rmdir(info.path, true, true, true);
    
            if (rmdir.isError()) {
              LOG(WARNING) << "Failed to delete '" << info.path << "': "
                           << rmdir.error();
              info.promise->fail(rmdir.error());
            } else {
              LOG(INFO) << "Deleted '" << info.path << "'";
              info.promise->set(rmdir.get());
            }
          }
        }
    
        async(rmdirs);
          .then(defer(self(),&Self::_remove, infos);
    ```
    
    Since our main objective is to move rmdir out of the `GarbageCollectorProcess` so perhaps
one async call to sequentially remove all of the paths as it was done before when it can simply
the code is worth it. In practice there likely won't be many paths sharing the same scheduled
gc time anyway.



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

    Blank line above.


- Jiang Yan Xu


On May 2, 2017, 2:33 p.m., Jacob Janco wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53479/
> -----------------------------------------------------------
> 
> (Updated May 2, 2017, 2:33 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/3/
> 
> 
> 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