mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 70214: Added skeleton code for v0 `VolumeManager`.
Date Fri, 22 Mar 2019 12:39:26 GMT


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


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