> On March 19, 2019, 3:57 p.m., Benjamin Bannier wrote: > > src/csi/v0_volume_manager.hpp > > Lines 100 (patched) > > <https://reviews.apache.org/r/70214/diff/2/?file=2132469#file2132469line100> > > > > Does it make sense to move this into the process? It would lead to > > simpler dispatch in the wrapper around the process, and probably be more in > > line with the way we usually lay this out. > > Chun-Hung Hsiao wrote: > Yes I agree doing what you suggested is more consistent with the existing > codebase. > > The reason I do this is because we can save one dispatch. If we move it > into the actor, it would be like: > ``` > Future<Nothing> VolumeManagerProcess::attachVolume(...) > { > return recovered > .then(defer(self(), [=] { ... })); > } > ``` > Since `recovered` might carry a future that is satisfied in another > actor. And we'll have: > ``` > Future<Nothing> VolumeManager::attachVolume(...) > { > return dispatch(process.get(), &VolumeProcessManager::attachVolume, > ...); > } > ``` > See, we'll always get two dispatches in a single `attachVolume` call. > > There are two ways to solve this: > 1. Keep `recovered` in the actor, but do the following: > ``` > // Ensure that the following future is satisfied in this actor. > recovered = recover().then(defer(self(), [] { return Nothing(); })); > ``` > So we could remove a `defer` in `attachVolume`. But the last `then` looks > a bit unnatural. > > 2. Do what I did in this patch. > > 3. Don't care about the two dispatches, since this is less likely to > introduce much overhead. > > WDYT? > > Benjamin Bannier wrote: > TBH, I don't really understand what (1) accomplishes as it seems that to > be able to drop the `defer` we'd need to assert that we are already recovered. > > I'd be in favor of (3) as it would just appear simpler and I do not > expect a lot of contention on the actor which makes optimizing this less > necessary, but I'll leave this up to you. > > Chun-Hung Hsiao wrote: > For (1), let's say we have the following code instead: > ``` > Future<Nothing> VolumeManagerProcess::attachVolume(...) > { > return recovered.then([=] { return _attachVolume(...); }); > } > ``` > if `recovered` is already completed, the continuation will be invoked > synchronously; if not, it will be invoked in the same actor where `recovered` > will be set. > That said, I will never want to write code like this ;) Just listing what > else can be done theoratically. > > A small benefit of (3) is that we don't need to introduce one more layer > of indentation in the actor method itself, and `FUTURE_DISPATCH` on the actor > function would be a bit more meaningful.
SGTM. > On March 19, 2019, 3:57 p.m., Benjamin Bannier wrote: > > src/csi/volume_manager.cpp > > Line 43 (original), 49 (patched) > > <https://reviews.apache.org/r/70214/diff/2/?file=2132472#file2132472line49> > > > > Same question as on previous review: how do you plan to do > > version-dependent dispatch here? > > Chun-Hung Hsiao wrote: > Something like: > ``` > // TODO(chhsiao): Do a move-capture with `Owned` once we get C++14. > auto serviceManager = make_shared(new Owned<ServiceManager>(new > ServiceManager(...))); > return (*serviceManager)->getCsiVersion() > .then([=](const string& version) -> Future<Owned<VolumeManager>> { > if (version == v1::VERSION) { > return new v1::VolumeManager(..., std::move(*serviceManager)); > } else if (version == v0::VERSION) { > return new v0::VolumeManager(..., std::move(*serviceManager)); > } > > UNREACHABLE(); > }); > ``` > > Benjamin Bannier wrote: > Looks good. > > Let's use a `shared_ptr` instead of an `Owned` so this code is > semantically correct. > > Chun-Hung Hsiao wrote: > Hmm I believe it is already semantically correct but it seems that `auto` > + `make_shared` is really not that readable ;) Maybe I should go with > ``` > std::shared_ptr<Owned<ServiceManager>> serviceManager(new > Owned<ServiceManager>(new ServiceManager(...))); > ``` > or > ``` > std::shared_ptr<Owned<ServiceManager>> serviceManager = > std::make_shared(new Owned<ServiceManager>(new ServiceManager(...)))); > > ``` > But both seem a bit less idomatic. > > The type of `serviceManager` is `std::shared_ptr<Owned<ServiceManager>>`. > The problem is that `std::shared_ptr` has no method to relinquish the > ownership without destroying the object. > > I can go with `std::shared_ptr<ServiceManager>` instead and change all > interfaces taking this argument to `std::shared_ptr` as well. But that means > in the future we'd like to do the changes again to switch back to eithor > `Owned` or `std::unique_ptr`. WDYT? I misread that, sorry. Could you update the `TODO`, e.g., ``` TODO(chhsia0): Do a move-capture of `Owned` and remove the `shared_ptr` use once we get C++14. ``` - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70214/#review213804 ----------------------------------------------------------- On March 22, 2019, 7:12 a.m., Chun-Hung Hsiao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70214/ > ----------------------------------------------------------- > > (Updated March 22, 2019, 7:12 a.m.) > > > Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht. > > > Bugs: MESOS-9622 > https://issues.apache.org/jira/browse/MESOS-9622 > > > Repository: mesos > > > Description > ------- > > Added skeleton code for v0 `VolumeManager`. > > > Diffs > ----- > > src/CMakeLists.txt 3397c3b1d4e8a7900b2e5f870679cc7aa30b4be2 > src/Makefile.am d451d7cabe3bf5d4f5039cfac5de1b03ef891d07 > src/csi/v0_volume_manager.hpp PRE-CREATION > src/csi/v0_volume_manager.cpp PRE-CREATION > src/csi/v0_volume_manager_process.hpp PRE-CREATION > src/csi/volume_manager.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/70214/diff/3/ > > > Testing > ------- > > make check > > > Thanks, > > Chun-Hung Hsiao > >
