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 70216: Implemented the recovery logic for v0 `VolumeManager`.
Date Wed, 20 Mar 2019 15:13:47 GMT

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



Let's squash the corresponding changes from https://reviews.apache.org/r/70222/ into this
patch.


src/csi/v0_volume_manager.cpp
Lines 157 (patched)
<https://reviews.apache.org/r/70216/#comment299933>

    `continue` for `None` case?



src/csi/v0_volume_manager.cpp
Lines 199 (patched)
<https://reviews.apache.org/r/70216/#comment299934>

    We could do an early return for invalid state to reduce nesting.



src/csi/v0_volume_manager.cpp
Lines 437 (patched)
<https://reviews.apache.org/r/70216/#comment299925>

    Let's assert that we have some `services`.



src/csi/v0_volume_manager.cpp
Lines 440 (patched)
<https://reviews.apache.org/r/70216/#comment299926>

    Should we have used an `Option` for this member?



src/csi/v0_volume_manager.cpp
Lines 452 (patched)
<https://reviews.apache.org/r/70216/#comment299917>

    `[this]`



src/csi/v0_volume_manager.cpp
Lines 454 (patched)
<https://reviews.apache.org/r/70216/#comment299924>

    Expand type, `CSIPluginContainerInfo::Service`.



src/csi/v0_volume_manager.cpp
Lines 455-456 (patched)
<https://reviews.apache.org/r/70216/#comment299927>

    This would be much easier to read if we had the function call all on a single line.



src/csi/v0_volume_manager.cpp
Lines 463 (patched)
<https://reviews.apache.org/r/70216/#comment299918>

    `[]`, this lambda doesn't need to execute on this process.



src/csi/v0_volume_manager.cpp
Lines 465 (patched)
<https://reviews.apache.org/r/70216/#comment299928>

    I was wondering what was special about `size==2`. Maybe expand to cover all sizes (same
`name`, `vendor_version`)?



src/csi/v0_volume_manager.cpp
Lines 478 (patched)
<https://reviews.apache.org/r/70216/#comment299919>

    `[this]`



src/csi/v0_volume_manager.cpp
Lines 485 (patched)
<https://reviews.apache.org/r/70216/#comment299920>

    `[this]`



src/csi/v0_volume_manager.cpp
Lines 487 (patched)
<https://reviews.apache.org/r/70216/#comment299929>

    Should we have an `Option` member instead?



src/csi/v0_volume_manager.cpp
Lines 492 (patched)
<https://reviews.apache.org/r/70216/#comment299921>

    `[this]`



src/csi/v0_volume_manager.cpp
Lines 499 (patched)
<https://reviews.apache.org/r/70216/#comment299922>

    `[this]`



src/csi/v0_volume_manager.cpp
Lines 501 (patched)
<https://reviews.apache.org/r/70216/#comment299930>

    `Option` member?



src/csi/v0_volume_manager.cpp
Lines 505 (patched)
<https://reviews.apache.org/r/70216/#comment299923>

    `[this]`



src/csi/v0_volume_manager.cpp
Lines 507 (patched)
<https://reviews.apache.org/r/70216/#comment299931>

    `Option`?



src/csi/v0_volume_manager_process.hpp
Lines 176 (patched)
<https://reviews.apache.org/r/70216/#comment299932>

    Can or should this be `const`?


- Benjamin Bannier


On March 15, 2019, 6:17 a.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70216/
> -----------------------------------------------------------
> 
> (Updated March 15, 2019, 6:17 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
> -------
> 
> The recovery logic is similar to SLRP's `recoverVolumes` method. But
> `VolumeManager` no longer recovers all volumes to their steady states
> since this is not necessary. Together with the new `publishVolume`
> design, the recovery logic is now simpler.
> 
> 
> Diffs
> -----
> 
>   src/csi/v0_volume_manager.cpp PRE-CREATION 
>   src/csi/v0_volume_manager_process.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70216/diff/2/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


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