You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jacob Janco <jj...@gmail.com> on 2016/12/02 02:58:15 UTC
Re: Review Request 53479: Perform agent GC asynchronously.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/
-----------------------------------------------------------
(Updated Dec. 2, 2016, 2:58 a.m.)
Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
Summary (updated)
-----------------
Perform agent GC asynchronously.
Bugs: MESOS-6549
https://issues.apache.org/jira/browse/MESOS-6549
Repository: mesos
Description (updated)
-------
- Previously, rmdir operations were serialized
through the garbage collector. Expensive removal
events had the potential to delay task launch.
- MESOS-6549
Diffs (updated)
-----
src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e
src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58
Diff: https://reviews.apache.org/r/53479/diff/
Testing
-------
`make check`
Thanks,
Jacob Janco
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jacob Janco <jj...@gmail.com>.
> On June 25, 2017, 7:33 a.m., Jiang Yan Xu wrote:
> > src/slave/gc.cpp
> > Line 153 (original), 149-178 (patched)
> > <https://reviews.apache.org/r/53479/diff/3/?file=1704584#file1704584line170>
> >
> > 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.
This makes sense to me, testing it, as this would avoid creating a bunch of new processes with an unknown bound.
- Jacob
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/#review178839
-----------------------------------------------------------
On July 11, 2017, 6:28 p.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53479/
> -----------------------------------------------------------
>
> (Updated July 11, 2017, 6:28 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/4/
>
>
> Testing
> -------
>
> `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
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
>
>
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/#review173727
-----------------------------------------------------------
Patch looks great!
Reviews applied: [53479]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On May 2, 2017, 9: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, 9: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
>
>
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/#review180280
-----------------------------------------------------------
Patch looks great!
Reviews applied: [53479]
Passed command: support\windows-build.bat
- Mesos Reviewbot Windows
On July 11, 2017, 6:28 p.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53479/
> -----------------------------------------------------------
>
> (Updated July 11, 2017, 6:28 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/4/
>
>
> Testing
> -------
>
> `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jacob Janco <jj...@gmail.com>.
> On July 12, 2017, 1:47 a.m., Jiang Yan Xu wrote:
> > src/slave/gc.cpp
> > Lines 179 (patched)
> > <https://reviews.apache.org/r/53479/diff/4/?file=1774291#file1774291line184>
> >
> > Better to call `onAny` and then `CHECK` that it's always ready in the continuation so the intention is explicit.
>
> Jacob Janco wrote:
> Checked this with a conditional...not sure when CHECK is appropriate.
>
> Jiang Yan Xu wrote:
> It's true that the lambda doesn't fail right? The point is that with onAny + CHECK_READY, it's clearer that we have covered all possible outcomes of the futrue and nothing is overlooked.
>
> However looking at it again, the lambda is relatively short and straightfoward so it's probably fine to drop this and continue to use onReady. onAny + CHECK_READY is fine too but no need to use `if` for which you need to handle cases that will never happen.
Ah ok, this is a libprocess check on the future. I'll use that to cover all cases.
- Jacob
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/#review180261
-----------------------------------------------------------
On July 13, 2017, 9: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, 9: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
>
>
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jacob Janco <jj...@gmail.com>.
> On July 12, 2017, 1:47 a.m., Jiang Yan Xu wrote:
> > src/slave/gc.hpp
> > Lines 102 (patched)
> > <https://reviews.apache.org/r/53479/diff/4/?file=1774290#file1774290line115>
> >
> > Indent less by two spaces.
Done.
> On July 12, 2017, 1:47 a.m., Jiang Yan Xu wrote:
> > src/slave/gc.hpp
> > Line 120 (original), 111-115 (patched)
> > <https://reviews.apache.org/r/53479/diff/4/?file=1774290#file1774290line124>
> >
> > Storing a `Owned<bool> removing` looks odd, instead we we can just have
> >
> > ```
> > struct PathInfo
> > {
> > ...
> >
> > process::Promise<Nothing> promise;
> > bool removing;
> > }
> > ```
> >
> > but store `Owned<PathInfo>` in
> >
> > ```
> > Multimap<process::Timeout, process::Owned<PathInfo>> paths;
> > ```
> >
> > ?
A+ comment, storing ptr instead.
> On July 12, 2017, 1:47 a.m., Jiang Yan Xu wrote:
> > src/slave/gc.cpp
> > Line 63 (original), 71 (patched)
> > <https://reviews.apache.org/r/53479/diff/4/?file=1774291#file1774291line71>
> >
> > We don't need to do these if we just store `Owned<PathInfo>`. In fact, it probably doesn't even need a custom constructor. It can be default constructed.
Agree, fixed.
> On July 12, 2017, 1:47 a.m., Jiang Yan Xu wrote:
> > src/slave/gc.cpp
> > Line 77 (original), 86 (patched)
> > <https://reviews.apache.org/r/53479/diff/4/?file=1774291#file1774291line86>
> >
> > This can be
> >
> > ```
> > return info->promise.future();
> > ```
> >
> > if you don't construct the promise outside PathInfo.
Fixed.
> On July 12, 2017, 1:47 a.m., Jiang Yan Xu wrote:
> > src/slave/gc.cpp
> > Lines 176-177 (patched)
> > <https://reviews.apache.org/r/53479/diff/4/?file=1774291#file1774291line181>
> >
> > IMO this is very obvious from the `async` call and the content os the labmda and probably no need for commenting.
Removed.
> On July 12, 2017, 1:47 a.m., Jiang Yan Xu wrote:
> > src/slave/gc.cpp
> > Lines 179 (patched)
> > <https://reviews.apache.org/r/53479/diff/4/?file=1774291#file1774291line184>
> >
> > Better to call `onAny` and then `CHECK` that it's always ready in the continuation so the intention is explicit.
Checked this with a conditional...not sure when CHECK is appropriate.
> On July 12, 2017, 1:47 a.m., Jiang Yan Xu wrote:
> > src/slave/gc.cpp
> > Lines 180-181 (patched)
> > <https://reviews.apache.org/r/53479/diff/4/?file=1774291#file1774291line185>
> >
> > Kill these lines.
Done.
> On July 12, 2017, 1:47 a.m., Jiang Yan Xu wrote:
> > src/slave/gc.cpp
> > Lines 193 (patched)
> > <https://reviews.apache.org/r/53479/diff/4/?file=1774291#file1774291line198>
> >
> > Put it above declaration?
Done.
> On July 12, 2017, 1:47 a.m., Jiang Yan Xu wrote:
> > src/slave/gc.cpp
> > Lines 194-195 (patched)
> > <https://reviews.apache.org/r/53479/diff/4/?file=1774291#file1774291line199>
> >
> > No need for passing both. In fact, `infos` is not used here when we should: there could be other entries of the same timeout added while the removal is ongoing but `infos` is the source of truth on what you just removed.
Indexed in by the timeouts entry from the path.
- Jacob
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/#review180261
-----------------------------------------------------------
On July 13, 2017, 9: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, 9: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
>
>
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jacob Janco <jj...@gmail.com>.
> On July 12, 2017, 1:47 a.m., Jiang Yan Xu wrote:
> > src/slave/gc.hpp
> > Line 108 (original), 97 (patched)
> > <https://reviews.apache.org/r/53479/diff/4/?file=1774290#file1774290line110>
> >
> > Why does this become public?
Fixed, moved to private.
> On July 12, 2017, 1:47 a.m., Jiang Yan Xu wrote:
> > src/slave/gc.hpp
> > Line 111 (original), 100 (patched)
> > <https://reviews.apache.org/r/53479/diff/4/?file=1774290#file1774290line113>
> >
> > Can this be const?
Can't `discard` a const.
> On July 12, 2017, 1:47 a.m., Jiang Yan Xu wrote:
> > src/slave/gc.hpp
> > Line 112 (original), 101 (patched)
> > <https://reviews.apache.org/r/53479/diff/4/?file=1774290#file1774290line114>
> >
> > Do we need to pass this in?
Initialized instead.
- Jacob
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/#review180261
-----------------------------------------------------------
On July 13, 2017, 9: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, 9: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
>
>
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On July 11, 2017, 6:47 p.m., Jiang Yan Xu wrote:
> > src/slave/gc.cpp
> > Lines 179 (patched)
> > <https://reviews.apache.org/r/53479/diff/4/?file=1774291#file1774291line184>
> >
> > Better to call `onAny` and then `CHECK` that it's always ready in the continuation so the intention is explicit.
>
> Jacob Janco wrote:
> Checked this with a conditional...not sure when CHECK is appropriate.
It's true that the lambda doesn't fail right? The point is that with onAny + CHECK_READY, it's clearer that we have covered all possible outcomes of the futrue and nothing is overlooked.
However looking at it again, the lambda is relatively short and straightfoward so it's probably fine to drop this and continue to use onReady. onAny + CHECK_READY is fine too but no need to use `if` for which you need to handle cases that will never happen.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/#review180261
-----------------------------------------------------------
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
>
>
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/#review180261
-----------------------------------------------------------
Looking good! Just some minor issues.
src/slave/gc.hpp
Line 108 (original), 97 (patched)
<https://reviews.apache.org/r/53479/#comment255344>
Why does this become public?
src/slave/gc.hpp
Line 111 (original), 100 (patched)
<https://reviews.apache.org/r/53479/#comment255342>
Can this be const?
src/slave/gc.hpp
Line 112 (original), 101 (patched)
<https://reviews.apache.org/r/53479/#comment255343>
Do we need to pass this in?
src/slave/gc.hpp
Lines 102 (patched)
<https://reviews.apache.org/r/53479/#comment255345>
Indent less by two spaces.
src/slave/gc.hpp
Line 120 (original), 111-115 (patched)
<https://reviews.apache.org/r/53479/#comment255347>
Storing a `Owned<bool> removing` looks odd, instead we we can just have
```
struct PathInfo
{
...
process::Promise<Nothing> promise;
bool removing;
}
```
but store `Owned<PathInfo>` in
```
Multimap<process::Timeout, process::Owned<PathInfo>> paths;
```
?
src/slave/gc.cpp
Line 63 (original), 71 (patched)
<https://reviews.apache.org/r/53479/#comment255348>
We don't need to do these if we just store `Owned<PathInfo>`. In fact, it probably doesn't even need a custom constructor. It can be default constructed.
src/slave/gc.cpp
Line 77 (original), 86 (patched)
<https://reviews.apache.org/r/53479/#comment255349>
This can be
```
return info->promise.future();
```
if you don't construct the promise outside PathInfo.
src/slave/gc.cpp
Lines 176-177 (patched)
<https://reviews.apache.org/r/53479/#comment255361>
IMO this is very obvious from the `async` call and the content os the labmda and probably no need for commenting.
src/slave/gc.cpp
Lines 179 (patched)
<https://reviews.apache.org/r/53479/#comment255360>
Better to call `onAny` and then `CHECK` that it's always ready in the continuation so the intention is explicit.
src/slave/gc.cpp
Lines 180-181 (patched)
<https://reviews.apache.org/r/53479/#comment255359>
Kill these lines.
src/slave/gc.cpp
Lines 193 (patched)
<https://reviews.apache.org/r/53479/#comment255362>
Put it above declaration?
src/slave/gc.cpp
Lines 194-195 (patched)
<https://reviews.apache.org/r/53479/#comment255363>
No need for passing both. In fact, `infos` is not used here when we should: there could be other entries of the same timeout added while the removal is ongoing but `infos` is the source of truth on what you just removed.
- Jiang Yan Xu
On July 11, 2017, 11:28 a.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53479/
> -----------------------------------------------------------
>
> (Updated July 11, 2017, 11:28 a.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/4/
>
>
> Testing
> -------
>
> `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jacob Janco <jj...@gmail.com>.
> On July 14, 2017, 8:47 a.m., Jiang Yan Xu wrote:
> > src/slave/gc.hpp
> > Lines 122-123 (patched)
> > <https://reviews.apache.org/r/53479/diff/6/?file=1776250#file1776250line123>
> >
> > No need for pointer?
> >
> > Just `process::Promise<Nothing> promise;` which is default initialized.
My mistake, I added an overload to Promise to help with the PathInfo check since we're not checking the pointer anymore.
> On July 14, 2017, 8:47 a.m., Jiang Yan Xu wrote:
> > src/slave/gc.cpp
> > Lines 150-152 (patched)
> > <https://reviews.apache.org/r/53479/diff/6/?file=1776251#file1776251line156>
> >
> > This comment doesn't seem necessary, we may have missed this initially but retrospectively resetting `removing` before starting async rmdir feels straighforward.
Cool, removing.
> On July 14, 2017, 8:47 a.m., Jiang Yan Xu wrote:
> > src/slave/gc.cpp
> > Lines 155 (patched)
> > <https://reviews.apache.org/r/53479/diff/6/?file=1776251#file1776251line161>
> >
> > Also add the action taken here, which is
> >
> > ```
> > VLOG(1) << "Skipping deletion of '" << info-> path << "' as it is already in progress";
> > ```
Fixed.
> On July 14, 2017, 8:47 a.m., Jiang Yan Xu wrote:
> > src/slave/gc.cpp
> > Lines 159 (patched)
> > <https://reviews.apache.org/r/53479/diff/6/?file=1776251#file1776251line165>
> >
> > 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?
That makes more sense, moving the log line there.
> On July 14, 2017, 8:47 a.m., Jiang Yan Xu wrote:
> > src/slave/gc.cpp
> > Lines 163 (patched)
> > <https://reviews.apache.org/r/53479/diff/6/?file=1776251#file1776251line169>
> >
> > I think the name and type `bool removing` is self-evident.
Removed comment.
> On July 14, 2017, 8:47 a.m., Jiang Yan Xu wrote:
> > src/slave/gc.cpp
> > Lines 205 (patched)
> > <https://reviews.apache.org/r/53479/diff/6/?file=1776251#file1776251line213>
> >
> > `CHECK_READY(result);`?
Added.
> On July 14, 2017, 8:47 a.m., Jiang Yan Xu wrote:
> > src/slave/gc.cpp
> > Lines 206 (patched)
> > <https://reviews.apache.org/r/53479/diff/6/?file=1776251#file1776251line214>
> >
> > Kill this since we have the CHECK_READY.
Removed.
> On July 14, 2017, 8:47 a.m., Jiang Yan Xu wrote:
> > src/slave/gc.cpp
> > Lines 210 (patched)
> > <https://reviews.apache.org/r/53479/diff/6/?file=1776251#file1776251line218>
> >
> > Use `CHECK_EQ(timeouts.erase(info->path), 1);`?
Also fixed this in `unschedule`
- Jacob
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/#review180510
-----------------------------------------------------------
On July 15, 2017, 1:20 a.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53479/
> -----------------------------------------------------------
>
> (Updated July 15, 2017, 1:20 a.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/8/
>
>
> Testing
> -------
>
> `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
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
>
>
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jacob Janco <jj...@gmail.com>.
> On July 17, 2017, 8:43 p.m., Jiang Yan Xu wrote:
> > src/slave/gc.hpp
> > Lines 114-117 (original), 115-118 (patched)
> > <https://reviews.apache.org/r/53479/diff/8/?file=1777247#file1777247line116>
> >
> > I see that `promise == that.promise` here is required by the Multimap but does it make sense to have `PathInfo` equality depend only on the path?
> >
> > I don't think `Promise` is supposed to be comparable.
>
> Jiang Yan Xu wrote:
> OK I think strictly speaking we shouldn't assume that there are no equal values under the same key in a Multimap (or std::multimap).
>
> This case shouldn't happen with our GC so we should probably insert some hard `CHECK`s for it (in a separate patch) but I don't think removing `promise == that.promise` has anything to do with it, i.e., the problem exists today.
>
> Jacob Janco wrote:
> Updating and removing the blocking dependency. Actually, I don't think we even use this operator...why not just remove this and add the checks like you said.
>
> Jiang Yan Xu wrote:
> I think it's required by Multimap.
Per discussion: Now that we store a pointer remove this equality.
- Jacob
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/#review180730
-----------------------------------------------------------
On July 18, 2017, 12:23 a.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53479/
> -----------------------------------------------------------
>
> (Updated July 18, 2017, 12:23 a.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/9/
>
>
> Testing
> -------
>
> `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jacob Janco <jj...@gmail.com>.
> On July 17, 2017, 8:43 p.m., Jiang Yan Xu wrote:
> > src/slave/gc.hpp
> > Lines 114-117 (original), 115-118 (patched)
> > <https://reviews.apache.org/r/53479/diff/8/?file=1777247#file1777247line116>
> >
> > I see that `promise == that.promise` here is required by the Multimap but does it make sense to have `PathInfo` equality depend only on the path?
> >
> > I don't think `Promise` is supposed to be comparable.
>
> Jiang Yan Xu wrote:
> OK I think strictly speaking we shouldn't assume that there are no equal values under the same key in a Multimap (or std::multimap).
>
> This case shouldn't happen with our GC so we should probably insert some hard `CHECK`s for it (in a separate patch) but I don't think removing `promise == that.promise` has anything to do with it, i.e., the problem exists today.
Updating and removing the blocking dependency. Actually, I don't think we even use this operator...why not just remove this and add the checks like you said.
- Jacob
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/#review180730
-----------------------------------------------------------
On July 15, 2017, 1:23 a.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53479/
> -----------------------------------------------------------
>
> (Updated July 15, 2017, 1:23 a.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
>
>
> Added: https://reviews.apache.org/r/60893/
>
>
> Diffs
> -----
>
> src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e
> src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58
>
>
> Diff: https://reviews.apache.org/r/53479/diff/8/
>
>
> Testing
> -------
>
> `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On July 17, 2017, 1:43 p.m., Jiang Yan Xu wrote:
> > src/slave/gc.hpp
> > Lines 114-117 (original), 115-118 (patched)
> > <https://reviews.apache.org/r/53479/diff/8/?file=1777247#file1777247line116>
> >
> > I see that `promise == that.promise` here is required by the Multimap but does it make sense to have `PathInfo` equality depend only on the path?
> >
> > I don't think `Promise` is supposed to be comparable.
OK I think strictly speaking we shouldn't assume that there are no equal values under the same key in a Multimap (or std::multimap).
This case shouldn't happen with our GC so we should probably insert some hard `CHECK`s for it (in a separate patch) but I don't think removing `promise == that.promise` has anything to do with it, i.e., the problem exists today.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/#review180730
-----------------------------------------------------------
On July 14, 2017, 6:23 p.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53479/
> -----------------------------------------------------------
>
> (Updated July 14, 2017, 6:23 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
>
>
> Added: https://reviews.apache.org/r/60893/
>
>
> Diffs
> -----
>
> src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e
> src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58
>
>
> Diff: https://reviews.apache.org/r/53479/diff/8/
>
>
> Testing
> -------
>
> `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On July 17, 2017, 1:43 p.m., Jiang Yan Xu wrote:
> > src/slave/gc.hpp
> > Lines 114-117 (original), 115-118 (patched)
> > <https://reviews.apache.org/r/53479/diff/8/?file=1777247#file1777247line116>
> >
> > I see that `promise == that.promise` here is required by the Multimap but does it make sense to have `PathInfo` equality depend only on the path?
> >
> > I don't think `Promise` is supposed to be comparable.
>
> Jiang Yan Xu wrote:
> OK I think strictly speaking we shouldn't assume that there are no equal values under the same key in a Multimap (or std::multimap).
>
> This case shouldn't happen with our GC so we should probably insert some hard `CHECK`s for it (in a separate patch) but I don't think removing `promise == that.promise` has anything to do with it, i.e., the problem exists today.
>
> Jacob Janco wrote:
> Updating and removing the blocking dependency. Actually, I don't think we even use this operator...why not just remove this and add the checks like you said.
I think it's required by Multimap.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/#review180730
-----------------------------------------------------------
On July 17, 2017, 5:23 p.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53479/
> -----------------------------------------------------------
>
> (Updated July 17, 2017, 5:23 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/9/
>
>
> Testing
> -------
>
> `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/#review180730
-----------------------------------------------------------
src/slave/gc.hpp
Lines 114-117 (original), 115-118 (patched)
<https://reviews.apache.org/r/53479/#comment255948>
I see that `promise == that.promise` here is required by the Multimap but does it make sense to have `PathInfo` equality depend only on the path?
I don't think `Promise` is supposed to be comparable.
src/slave/gc.cpp
Line 100 (original), 119 (patched)
<https://reviews.apache.org/r/53479/#comment255950>
`s/(unsigned long)1/1u/`
src/slave/gc.cpp
Lines 209 (patched)
<https://reviews.apache.org/r/53479/#comment255951>
`s/(unsigned long)1/1u/`
- Jiang Yan Xu
On July 14, 2017, 6:23 p.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53479/
> -----------------------------------------------------------
>
> (Updated July 14, 2017, 6:23 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
>
>
> Added: https://reviews.apache.org/r/60893/
>
>
> Diffs
> -----
>
> src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e
> src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58
>
>
> Diff: https://reviews.apache.org/r/53479/diff/8/
>
>
> Testing
> -------
>
> `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/#review180630
-----------------------------------------------------------
Bad review!
Reviews applied: [53479]
Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing list.
- Mesos Reviewbot Windows
On July 14, 2017, 9:23 p.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53479/
> -----------------------------------------------------------
>
> (Updated July 14, 2017, 9:23 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
>
>
> Added: https://reviews.apache.org/r/60893/
>
>
> Diffs
> -----
>
> src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e
> src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58
>
>
> Diff: https://reviews.apache.org/r/53479/diff/8/
>
>
> Testing
> -------
>
> `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jacob Janco <jj...@gmail.com>.
> On July 18, 2017, 10:50 p.m., Jiang Yan Xu wrote:
> > Committing with these additional edits.
Awesome, thanks!
- Jacob
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/#review180877
-----------------------------------------------------------
On July 18, 2017, 10:15 p.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53479/
> -----------------------------------------------------------
>
> (Updated July 18, 2017, 10:15 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/10/
>
>
> Testing
> -------
>
> `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/#review180877
-----------------------------------------------------------
Ship it!
Committing with these additional edits.
src/slave/gc.cpp
Lines 20 (patched)
<https://reviews.apache.org/r/53479/#comment256192>
We don't need this anymore.
src/slave/gc.cpp
Lines 25 (patched)
<https://reviews.apache.org/r/53479/#comment256194>
This should be below foreach.hpp.
src/slave/gc.cpp
Lines 202 (patched)
<https://reviews.apache.org/r/53479/#comment256196>
Included `#include <process/check.hpp>` for this.
src/slave/gc.cpp
Lines 204 (patched)
<https://reviews.apache.org/r/53479/#comment256191>
Removed this logging as it doesn't provide much additional info on top of existing logging, other than the fact that some callback has been called.
- Jiang Yan Xu
On July 18, 2017, 3:15 p.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53479/
> -----------------------------------------------------------
>
> (Updated July 18, 2017, 3:15 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/10/
>
>
> Testing
> -------
>
> `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jacob Janco <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/
-----------------------------------------------------------
(Updated July 18, 2017, 10:15 p.m.)
Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
Changes
-------
Final.
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 (updated)
-----
src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e
src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58
Diff: https://reviews.apache.org/r/53479/diff/10/
Changes: https://reviews.apache.org/r/53479/diff/9-10/
Testing
-------
`./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
Thanks,
Jacob Janco
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jacob Janco <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/
-----------------------------------------------------------
(Updated July 18, 2017, 12:23 a.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/9/
Testing
-------
`./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
Thanks,
Jacob Janco
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jacob Janco <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/
-----------------------------------------------------------
(Updated July 18, 2017, 12:22 a.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 (updated)
-------
- Previously, rmdir operations were serialized
through the garbage collector. Expensive removal
events had the potential to delay task launch.
- MESOS-6549
Diffs (updated)
-----
src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e
src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58
Diff: https://reviews.apache.org/r/53479/diff/9/
Changes: https://reviews.apache.org/r/53479/diff/8-9/
Testing
-------
`./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
Thanks,
Jacob Janco
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jacob Janco <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/
-----------------------------------------------------------
(Updated July 15, 2017, 1:23 a.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 (updated)
-------
- Previously, rmdir operations were serialized
through the garbage collector. Expensive removal
events had the potential to delay task launch.
- MESOS-6549
Added: https://reviews.apache.org/r/60893/
Diffs
-----
src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e
src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58
Diff: https://reviews.apache.org/r/53479/diff/8/
Testing
-------
`./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
Thanks,
Jacob Janco
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jacob Janco <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/
-----------------------------------------------------------
(Updated July 15, 2017, 1:20 a.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 (updated)
-----
src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e
src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58
Diff: https://reviews.apache.org/r/53479/diff/8/
Changes: https://reviews.apache.org/r/53479/diff/7-8/
Testing
-------
`./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
Thanks,
Jacob Janco
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jacob Janco <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/
-----------------------------------------------------------
(Updated July 15, 2017, 1:15 a.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 (updated)
-----
src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e
src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58
Diff: https://reviews.apache.org/r/53479/diff/7/
Changes: https://reviews.apache.org/r/53479/diff/6-7/
Testing
-------
`./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
Thanks,
Jacob Janco
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/#review180485
-----------------------------------------------------------
Patch looks great!
Reviews applied: [53479]
Passed command: support\windows-build.bat
- Mesos Reviewbot Windows
On July 13, 2017, 5: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, 5: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
>
>
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jacob Janco <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/
-----------------------------------------------------------
(Updated July 13, 2017, 9:48 p.m.)
Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
Changes
-------
remove log line.
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 (updated)
-----
src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e
src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58
Diff: https://reviews.apache.org/r/53479/diff/6/
Changes: https://reviews.apache.org/r/53479/diff/5-6/
Testing
-------
`./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
Thanks,
Jacob Janco
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jacob Janco <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/
-----------------------------------------------------------
(Updated July 13, 2017, 9:43 p.m.)
Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
Changes
-------
Fixed a bug in paths/timeouts cleanup + review comment fixes.
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 (updated)
-----
src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e
src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58
Diff: https://reviews.apache.org/r/53479/diff/5/
Changes: https://reviews.apache.org/r/53479/diff/4-5/
Testing
-------
`./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
Thanks,
Jacob Janco
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jacob Janco <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/
-----------------------------------------------------------
(Updated July 11, 2017, 6:28 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 (updated)
-----
src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e
src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58
Diff: https://reviews.apache.org/r/53479/diff/4/
Changes: https://reviews.apache.org/r/53479/diff/3-4/
Testing
-------
`./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
Thanks,
Jacob Janco
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jacob Janco <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/
-----------------------------------------------------------
(Updated May 2, 2017, 9:33 p.m.)
Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
Changes
-------
Rebase on upstream.
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 (updated)
-----
src/slave/gc.hpp 5ea82456cffa374203371f227b6f0ee00296553e
src/slave/gc.cpp 961f547793984449ea113d9664b12b5033638c58
Diff: https://reviews.apache.org/r/53479/diff/3/
Changes: https://reviews.apache.org/r/53479/diff/2-3/
Testing
-------
`./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
Thanks,
Jacob Janco
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jacob Janco <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/
-----------------------------------------------------------
(Updated May 1, 2017, 8:42 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/2/
Testing
-------
`./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
Thanks,
Jacob Janco
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/#review167497
-----------------------------------------------------------
Closing this review due to inactivity. Please see our [guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md) for reopening reviews.
- Joris Van Remoortere
On Dec. 2, 2016, 3:01 a.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53479/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2016, 3:01 a.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/2/
>
>
> Testing
> -------
>
> `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/#review157731
-----------------------------------------------------------
Patch looks great!
Reviews applied: [53479]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On Dec. 2, 2016, 3:01 a.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53479/
> -----------------------------------------------------------
>
> (Updated Dec. 2, 2016, 3:01 a.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/
>
>
> Testing
> -------
>
> `./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 53479: Perform agent GC asynchronously.
Posted by Jacob Janco <jj...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53479/
-----------------------------------------------------------
(Updated Dec. 2, 2016, 3:01 a.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/
Testing (updated)
-------
`./bin/mesos-tests.sh --gtest_filter="GarbageCollector*" --gtest_repeat=100 --gtest_break_on_failure`
Thanks,
Jacob Janco