----------------------------------------------------------- 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 > >
