You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Chun-Hung Hsiao <ch...@apache.org> on 2019/03/15 05:15:36 UTC
Review Request 70214: Added skeleton code for v0 `VolumeManager`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70214/
-----------------------------------------------------------
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/1/
Testing
-------
make check
Thanks,
Chun-Hung Hsiao
Re: Review Request 70214: Added skeleton code for v0 `VolumeManager`.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70214/
-----------------------------------------------------------
(Updated March 27, 2019, 5:56 a.m.)
Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
Changes
-------
Rebased.
Bugs: MESOS-9622
https://issues.apache.org/jira/browse/MESOS-9622
Repository: mesos
Description
-------
Added skeleton code for v0 `VolumeManager`.
Diffs (updated)
-----
src/CMakeLists.txt 3397c3b1d4e8a7900b2e5f870679cc7aa30b4be2
src/Makefile.am bcafe48b2105575371464a29783bc6f3f1c2cf8d
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/5/
Changes: https://reviews.apache.org/r/70214/diff/4-5/
Testing
-------
make check
Thanks,
Chun-Hung Hsiao
Re: Review Request 70214: Added skeleton code for v0 `VolumeManager`.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70214/
-----------------------------------------------------------
(Updated March 26, 2019, 5:06 a.m.)
Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
Changes
-------
Reorder the header inclusion correctly.
Bugs: MESOS-9622
https://issues.apache.org/jira/browse/MESOS-9622
Repository: mesos
Description
-------
Added skeleton code for v0 `VolumeManager`.
Diffs (updated)
-----
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/4/
Changes: https://reviews.apache.org/r/70214/diff/3-4/
Testing
-------
make check
Thanks,
Chun-Hung Hsiao
Re: Review Request 70214: Added skeleton code for v0 `VolumeManager`.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70214/#review213975
-----------------------------------------------------------
Ship it!
- Benjamin Bannier
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
>
>
Re: Review Request 70214: Added skeleton code for v0 `VolumeManager`.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
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.
Changes
-------
Addressed Benjamin's comments.
Bugs: MESOS-9622
https://issues.apache.org/jira/browse/MESOS-9622
Repository: mesos
Description
-------
Added skeleton code for v0 `VolumeManager`.
Diffs (updated)
-----
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/
Changes: https://reviews.apache.org/r/70214/diff/2-3/
Testing
-------
make check
Thanks,
Chun-Hung Hsiao
Re: Review Request 70214: Added skeleton code for v0 `VolumeManager`.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> 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
>
>
Re: Review Request 70214: Added skeleton code for v0 `VolumeManager`.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> 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
>
>
Re: Review Request 70214: Added skeleton code for v0 `VolumeManager`.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> 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
>
>
Re: Review Request 70214: Added skeleton code for v0 `VolumeManager`.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> 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
>
>
Re: Review Request 70214: Added skeleton code for v0 `VolumeManager`.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70214/#review213804
-----------------------------------------------------------
This review does seem to do exactly what https://reviews.apache.org/r/70213/ started, but not actually implement any functionality either. Squashing them both wouldn't be confusing I think.
src/csi/v0_volume_manager.hpp
Lines 57 (patched)
<https://reviews.apache.org/r/70214/#comment299832>
Did you intend to take a ref here, or `move` when initializing the member below? The former would be consistent to the treatment of other arguments.
src/csi/v0_volume_manager.hpp
Lines 100 (patched)
<https://reviews.apache.org/r/70214/#comment299852>
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.
src/csi/v0_volume_manager.cpp
Lines 70 (patched)
<https://reviews.apache.org/r/70214/#comment299844>
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?
src/csi/v0_volume_manager.cpp
Lines 76 (patched)
<https://reviews.apache.org/r/70214/#comment299853>
Shouldn't we be able to dispatch to the process for all of these already like we already do for some methods below?
I'd say either dispatch to all existing process methods, or don't even bother declaring unused methods in the process.
src/csi/v0_volume_manager.cpp
Lines 161 (patched)
<https://reviews.apache.org/r/70214/#comment299845>
The `CHECK_NOTNULL` seems redundant here, but doesn't hurt.
src/csi/v0_volume_manager.cpp
Lines 181 (patched)
<https://reviews.apache.org/r/70214/#comment299846>
We usually wrap before `.then` (and probably should update our `clang-format` fork to do it as well).
src/csi/v0_volume_manager.cpp
Lines 189 (patched)
<https://reviews.apache.org/r/70214/#comment299847>
Ditto.
src/csi/v0_volume_manager.cpp
Lines 218 (patched)
<https://reviews.apache.org/r/70214/#comment299848>
Ditto.
src/csi/v0_volume_manager.cpp
Lines 229 (patched)
<https://reviews.apache.org/r/70214/#comment299849>
Ditto.
src/csi/v0_volume_manager.cpp
Lines 243 (patched)
<https://reviews.apache.org/r/70214/#comment299850>
Ditto.
src/csi/v0_volume_manager.cpp
Lines 257 (patched)
<https://reviews.apache.org/r/70214/#comment299851>
Ditto.
src/csi/v0_volume_manager_process.hpp
Lines 49 (patched)
<https://reviews.apache.org/r/70214/#comment299843>
Why can't we define this in `src/csi/v0_volume_manager.hpp` as we usually do?
src/csi/v0_volume_manager_process.hpp
Lines 98 (patched)
<https://reviews.apache.org/r/70214/#comment299854>
Let's make this class non-copyable referencing MESOS-5122.
src/csi/volume_manager.cpp
Line 43 (original), 49 (patched)
<https://reviews.apache.org/r/70214/#comment299842>
Same question as on previous review: how do you plan to do version-dependent dispatch here?
- Benjamin Bannier
On March 15, 2019, 6: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, 6: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
>
>