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 70428: Auto-detect CSI API version to create the proper volume manager.
Date Tue, 09 Apr 2019 09:50:19 GMT

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


Fix it, then Ship it!





src/csi/service_manager.cpp
Lines 360 (patched)
<https://reviews.apache.org/r/70428/#comment300704>

    Any reason we couldn't return early here if `apiVersion.isSome()`?



src/csi/service_manager.cpp
Lines 362-363 (patched)
<https://reviews.apache.org/r/70428/#comment300705>

    It's a little weird that we have to invoke such a heavy function here which _as a side
effect_ sets the member, but that might be exactly how the csi interface was intended to use.



src/csi/volume_manager.cpp
Line 50 (original), 55 (patched)
<https://reviews.apache.org/r/70428/#comment300706>

    Let's construct a value of the correct type so we can leverage RVO,
    
    ```
    return Try<Owned<VolumeManager>>(new v0::VolumeManager(
        ...
    ```
    
    Here and below.


- Benjamin Bannier


On April 9, 2019, 8:08 a.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70428/
> -----------------------------------------------------------
> 
> (Updated April 9, 2019, 8:08 a.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Bugs: MESOS-9626
>     https://issues.apache.org/jira/browse/MESOS-9626
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The `mesos::csi::VolumeManager::create` function now first creates a
> `ServiceManager` to detect the API version, then instantiate the proper
> `VolumeManager` based on it.
> 
> NOTE: This patch enables CSI v1 by default for all SLRP-related unit
> tests except for test `RetryRpcWithExponentialBackoff`.
> 
> 
> Diffs
> -----
> 
>   src/csi/service_manager.hpp 5dd6bc7e044670c7416e2fc452ef46a2a50a6cf5 
>   src/csi/service_manager.cpp 0a3663cfd0dae2672a11eebcb6ffa3f8fad68ae0 
>   src/csi/v0_volume_manager.hpp 6c15f290389dfa150edf073c6033f0580e2e32b3 
>   src/csi/v0_volume_manager.cpp 7de900095bb86c207869c599c17ca08838e722be 
>   src/csi/v0_volume_manager_process.hpp 88073e423559f1fc5d171dbbac26e46472589e5a 
>   src/csi/v1_volume_manager.hpp PRE-CREATION 
>   src/csi/v1_volume_manager.cpp PRE-CREATION 
>   src/csi/v1_volume_manager_process.hpp PRE-CREATION 
>   src/csi/volume_manager.hpp cc20f461868b6b578b7e55ca0df66fbe26d8cd49 
>   src/csi/volume_manager.cpp cbe45cb439defbdd22bc57648b455e69679bdb4f 
>   src/resource_provider/storage/provider.cpp 2dc5c26b11f3fff179b4357b0dcc149e93d4e12f

>   src/tests/storage_local_resource_provider_tests.cpp da8db41b867d04b4236ad3515811fc00318c619c

> 
> 
> Diff: https://reviews.apache.org/r/70428/diff/2/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


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