You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Chun-Hung Hsiao <ch...@apache.org> on 2019/05/31 03:11:08 UTC
Review Request 70766: Fixed chaining futures infinitely in
`UriDiskProfileAdaptor`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70766/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier and Benjamin Mahler.
Bugs: MESOS-9803
https://issues.apache.org/jira/browse/MESOS-9803
Repository: mesos
Description
-------
Previously it is possible to have an infinite chain of futures when
`UriDiskProfileAdaptor::watch` is called: if the set of profiles remains
fixed for every poll, each poll would satisfy a promise that triggers
an asynchronous recursive call to `UriDiskProfileAdaptor::watch` again.
This patch fixes the problem by removing the asynchronous recursion.
Instead, we maintain a separated promise for each watcher that is never
associated to another promise. After each poll, we check if the current
set of profiles differs from the known set for a watcher, and satisfy
its own promise if so.
Diffs
-----
src/resource_provider/storage/uri_disk_profile_adaptor.hpp a5a34dc4dc1d518dc69aeb15fe62bd124d828ed3
src/resource_provider/storage/uri_disk_profile_adaptor.cpp 215f7f92b5c2a0e60555134ce9887e8a187e3b1d
Diff: https://reviews.apache.org/r/70766/diff/1/
Testing
-------
make check
Manually tested with the unit test in r/70764. The unit test will fail at the 5th poll without the fix and will pass with the fix.
Thanks,
Chun-Hung Hsiao
Re: Review Request 70766: Fixed chaining futures infinitely in
`UriDiskProfileAdaptor`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70766/#review215658
-----------------------------------------------------------
src/resource_provider/storage/uri_disk_profile_adaptor.hpp
Lines 261 (patched)
<https://reviews.apache.org/r/70766/#comment302444>
Just use a vector? Since this likely will remain small and you linearly scan it anyway to find the iterator position to erase, list doesn't seem to have benefit here.
It also looks like all the set construction / comparision will be vastly more expensive than the list / vector operations here FWIW.
src/resource_provider/storage/uri_disk_profile_adaptor.cpp
Lines 293 (patched)
<https://reviews.apache.org/r/70766/#comment302445>
I'm a little unsure of the order of evaluation here that ensures that the watcher is incremented before the erase call completes. If that doesn't hold, then incrementing the watcher will be undefined behavior since erase will invalidate it.
If you switch to vector, you can just use indexes?
```
size_t index = 0u;
while (index < watchers.size()) {
hashset<string> current;
foreachpair (
const string& profile, const ProfileRecord& record, profileMatrix) {
if (record.active &&
isSelectedResourceProvider(record.manifest, watchers[index].info)) {
current.insert(profile);
}
}
if (current != watcher->known) {
CHECK(watchers[index].promise.set(current))
<< "Promise for watcher '" << watchers[index].info << "' is already "
<< watcher->promise.future();
watchers.erase(index);
} else {
++index;
}
}
```
Probably storing `watcher` in a variable instead of all the indexing.
- Benjamin Mahler
On May 31, 2019, 3:11 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70766/
> -----------------------------------------------------------
>
> (Updated May 31, 2019, 3:11 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Bugs: MESOS-9803
> https://issues.apache.org/jira/browse/MESOS-9803
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously it is possible to have an infinite chain of futures when
> `UriDiskProfileAdaptor::watch` is called: if the set of profiles remains
> fixed for every poll, each poll would satisfy a promise that triggers
> an asynchronous recursive call to `UriDiskProfileAdaptor::watch` again.
>
> This patch fixes the problem by removing the asynchronous recursion.
> Instead, we maintain a separated promise for each watcher that is never
> associated to another promise. After each poll, we check if the current
> set of profiles differs from the known set for a watcher, and satisfy
> its own promise if so.
>
>
> Diffs
> -----
>
> src/resource_provider/storage/uri_disk_profile_adaptor.hpp a5a34dc4dc1d518dc69aeb15fe62bd124d828ed3
> src/resource_provider/storage/uri_disk_profile_adaptor.cpp 215f7f92b5c2a0e60555134ce9887e8a187e3b1d
>
>
> Diff: https://reviews.apache.org/r/70766/diff/1/
>
>
> Testing
> -------
>
> make check
>
> Manually tested with the unit test in r/70764. The unit test will fail at the 5th poll without the fix and will pass with the fix.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 70766: Fixed chaining futures infinitely in
`UriDiskProfileAdaptor`.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70766/#review215662
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Mahler
On May 31, 2019, 3:11 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70766/
> -----------------------------------------------------------
>
> (Updated May 31, 2019, 3:11 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Bugs: MESOS-9803
> https://issues.apache.org/jira/browse/MESOS-9803
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously it is possible to have an infinite chain of futures when
> `UriDiskProfileAdaptor::watch` is called: if the set of profiles remains
> fixed for every poll, each poll would satisfy a promise that triggers
> an asynchronous recursive call to `UriDiskProfileAdaptor::watch` again.
>
> This patch fixes the problem by removing the asynchronous recursion.
> Instead, we maintain a separated promise for each watcher that is never
> associated to another promise. After each poll, we check if the current
> set of profiles differs from the known set for a watcher, and satisfy
> its own promise if so.
>
>
> Diffs
> -----
>
> src/resource_provider/storage/uri_disk_profile_adaptor.hpp a5a34dc4dc1d518dc69aeb15fe62bd124d828ed3
> src/resource_provider/storage/uri_disk_profile_adaptor.cpp 215f7f92b5c2a0e60555134ce9887e8a187e3b1d
>
>
> Diff: https://reviews.apache.org/r/70766/diff/1/
>
>
> Testing
> -------
>
> make check
>
> Manually tested with the unit test in r/70764. The unit test will fail at the 5th poll without the fix and will pass with the fix.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 70766: Fixed chaining futures infinitely in
`UriDiskProfileAdaptor`.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70766/
-----------------------------------------------------------
(Updated June 6, 2019, 4:58 a.m.)
Review request for mesos, Benjamin Bannier and Benjamin Mahler.
Changes
-------
Made `watchers` a vector and avoided the use of iterators.
Bugs: MESOS-9803
https://issues.apache.org/jira/browse/MESOS-9803
Repository: mesos
Description
-------
Previously it is possible to have an infinite chain of futures when
`UriDiskProfileAdaptor::watch` is called: if the set of profiles remains
fixed for every poll, each poll would satisfy a promise that triggers
an asynchronous recursive call to `UriDiskProfileAdaptor::watch` again.
This patch fixes the problem by removing the asynchronous recursion.
Instead, we maintain a separated promise for each watcher that is never
associated to another promise. After each poll, we check if the current
set of profiles differs from the known set for a watcher, and satisfy
its own promise if so.
Diffs (updated)
-----
src/resource_provider/storage/uri_disk_profile_adaptor.hpp a5a34dc4dc1d518dc69aeb15fe62bd124d828ed3
src/resource_provider/storage/uri_disk_profile_adaptor.cpp 215f7f92b5c2a0e60555134ce9887e8a187e3b1d
Diff: https://reviews.apache.org/r/70766/diff/2/
Changes: https://reviews.apache.org/r/70766/diff/1-2/
Testing
-------
make check
Manually tested with the unit test in r/70764. The unit test will fail at the 5th poll without the fix and will pass with the fix.
Thanks,
Chun-Hung Hsiao