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 Wed, 20 Mar 2019 22:21:27 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.

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?


> On March 19, 2019, 2:57 p.m., Benjamin Bannier wrote:
> > src/csi/v0_volume_manager.cpp
> > Lines 70 (patched)
> > <https://reviews.apache.org/r/70214/diff/2/?file=2132470#file2132470line70>
> >
> >     Let's add a log string here as this isn't an internal invariant.
> >     
> >     Is this something which should have been defined as a precondition in the interface?

Good point. The validation happens in `VolumeManager::create` so let me add some comments
and logs.


> On March 19, 2019, 2:57 p.m., Benjamin Bannier wrote:
> > src/csi/v0_volume_manager_process.hpp
> > Lines 49 (patched)
> > <https://reviews.apache.org/r/70214/diff/2/?file=2132471#file2132471line49>
> >
> >     Why can't we define this in `src/csi/v0_volume_manager.hpp` as we usually do?

I believe you're talking about `v0_volume_manager.cpp`. This is because some SLRP tests uses
`FUTURE_DISPATCH(__call)` to synchronize test progress so they need to be exposed. Because
of the code movement, I unexpose `StorageLocalResourceProviderProcess` in r/70223. But I'm
not sure if we should land that one, so feel free to discard that patch.


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

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();
  });
```


- Chun-Hung


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


On March 15, 2019, 5:15 a.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70214/
> -----------------------------------------------------------
> 
> (Updated March 15, 2019, 5:15 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/2/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


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