> 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?
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. > 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(); > }); > ``` Looks good. Let's use a `shared_ptr` instead of an `Owned` so this code is semantically correct. - 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 > >