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 70214: Added skeleton code for v0 `VolumeManager`.
Date Tue, 19 Mar 2019 14:57:43 GMT

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


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