mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 70766: Fixed chaining futures infinitely in `UriDiskProfileAdaptor`.
Date Mon, 03 Jun 2019 19:41:11 GMT

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


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