> On Jan. 25, 2019, 3:05 p.m., Benjamin Bannier wrote: > > src/resource_provider/storage/provider.cpp > > Lines 1920-1922 (patched) > > <https://reviews.apache.org/r/69812/diff/2/?file=2121717#file2121717line1920> > > > > Does this actually increase the backoff every time? I guess the > > intention is that we pass a `mutable` backoff by value, and then increase > > it every time. I have the feeling this only works on the copy and doesn't > > propagate across iterations. > > > > I would have expected some function-local `shared_ptr` to a backoff > > which we'd pass by value into the `loop` body as `mutable` and can use to > > pass the value across iterations. Not sure there is a dedicated `loop` > > solution for this.
I printed out logs to check this ;) Also I had an old not-submitted-yet patch that verified this would work: https://reviews.apache.org/r/60846/diff/5#4 In fact, the `process::internal::Loop` class holds a copy of the callable passed in, and always call it on the same copy: https://github.com/apache/mesos/blob/master/3rdparty/libprocess/include/process/loop.hpp#L335 I had a discussion with BenM before and we actually used this in the codebase: https://github.com/apache/mesos/blob/master/src/slave/http.cpp#L3367 Also, you might have known this, but just FYI, a `const std::function<...>&` can wrap a mutable lambda and call it without a problem. Another nasty part in C++ ;) I originally thought about using fixed exponential backoff so we can precisely test that in the unit test. But, the original problem we're trying to resolve is to deal with scenarios where a number of operations suddenly appear, and with a fixed exponential backoff their retries would still be synchronized, so decided to go with a random backoff. Dropping this as there is no problem with this implementation. > On Jan. 25, 2019, 3:05 p.m., Benjamin Bannier wrote: > > src/resource_provider/storage/provider.cpp > > Line 1901 (original), 1959 (patched) > > <https://reviews.apache.org/r/69812/diff/2/?file=2121717#file2121717line1960> > > > > Do we want to include abandoned futures here? `onAny` is for terminal futures and wouldn't be called when the future is transitioned to `ABANDONED`: https://github.com/apache/mesos/blob/master/3rdparty/libprocess/include/process/future.hpp#L1182 Also, the future should never be abandoned, since the completion queue would hold a reference to the promise until the call has been dequeued. Dropping. - Chun-Hung ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69812/#review212322 ----------------------------------------------------------- On Jan. 23, 2019, 7:10 a.m., Chun-Hung Hsiao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69812/ > ----------------------------------------------------------- > > (Updated Jan. 23, 2019, 7:10 a.m.) > > > Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and Jan > Schlicht. > > > Bugs: MESOS-9517 > https://issues.apache.org/jira/browse/MESOS-9517 > > > Repository: mesos > > > Description > ------- > > When the CSI plugin returns a retryable error (i.e., `DEADLINE_EXCEEDED` > or `UNAVAILABLE`) for `CreateVolume` or `DeleteVolume` CSI calls, SLRP > will now retry indefinitely with a random exponential backoff. > > > Diffs > ----- > > src/csi/client.hpp 5d40d54c2abbd03993ce8835d37db23e209c7554 > src/csi/client.cpp 61ed410985099828a2f58b1527ab57daa4b379df > src/resource_provider/storage/provider.hpp > 331f7b785b14b814c2889488effd53f3a48a1b95 > src/resource_provider/storage/provider.cpp > d6e20a549ede189c757ae3ae922ab7cb86d2be2c > > > Diff: https://reviews.apache.org/r/69812/diff/2/ > > > Testing > ------- > > make check > > A unit test will be added later in the chain. > > > Thanks, > > Chun-Hung Hsiao > >
