mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chun-Hung Hsiao <chhs...@apache.org>
Subject Re: Review Request 70214: Added skeleton code for v0 `VolumeManager`.
Date Fri, 22 Mar 2019 19:03:20 GMT


> On March 19, 2019, 2: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.

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.


> On March 19, 2019, 2: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.

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?


- Chun-Hung


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70214/#review213804
-----------------------------------------------------------


On March 22, 2019, 6: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, 6: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