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 69787: Added a proxy mode to the test CSI plugin.
Date Fri, 18 Jan 2019 11:03:15 GMT

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




src/examples/test_csi_plugin.cpp
Lines 115 (patched)
<https://reviews.apache.org/r/69787/#comment297769>

    Could we add an example here?



src/examples/test_csi_plugin.cpp
Lines 925 (patched)
<https://reviews.apache.org/r/69787/#comment297775>

    Could you add a comment here explaining its purpose?



src/examples/test_csi_plugin.cpp
Lines 930-931 (patched)
<https://reviews.apache.org/r/69787/#comment297774>

    Just accept values here? Move construction of `unique_ptr` is cheap and possibly elided.



src/examples/test_csi_plugin.cpp
Lines 956-963 (patched)
<https://reviews.apache.org/r/69787/#comment297776>

    Reordering these would make a little clearer what we have here, e.g., the following or
sim.
    
         GenericServerContext serverContext;
         GenericServerAsyncReaderWriter serverReaderWriter;
         ClientContext clientContext;
         std::unique_ptr<GenericClientAsyncResponseReader> clientReader;
    
         State state;
    
         ByteBuffer request;
    
         ByteBuffer response;
         Status status;



src/examples/test_csi_plugin.cpp
Lines 970 (patched)
<https://reviews.apache.org/r/69787/#comment297777>

    Could we give this a better name, e.g., `completionQueue`? That would be less gRPC'y,
but more Mesos'y.



src/examples/test_csi_plugin.cpp
Lines 975 (patched)
<https://reviews.apache.org/r/69787/#comment297781>

    Could you add a comment somewhere documenting the state machine? Probably here instead
of in `Call`.



src/examples/test_csi_plugin.cpp
Lines 978 (patched)
<https://reviews.apache.org/r/69787/#comment297770>

    Let's use a smart pointer to make clear that we pass ownership below, e.g.,
    
        std::unique_ptr<Call> first(new Call);
        
        // Pass ownership below.
        ... first.release() ...
        
    (Btw, we pull `unique_ptr` into the current scope with a using decl, but still spell it
out in full in most places).



src/examples/test_csi_plugin.cpp
Lines 990 (patched)
<https://reviews.apache.org/r/69787/#comment297771>

    We should log something here as this could fail in general.



src/examples/test_csi_plugin.cpp
Lines 999 (patched)
<https://reviews.apache.org/r/69787/#comment297772>

    Use a smart pointer.



src/examples/test_csi_plugin.cpp
Lines 1015-1016 (patched)
<https://reviews.apache.org/r/69787/#comment297773>

    Spell out grpc types here?



src/examples/test_csi_plugin.cpp
Line 1020 (original), 1146 (patched)
<https://reviews.apache.org/r/69787/#comment297779>

    I was wondering whether one would need to explicitly shut down the server and completion
queue, or does that happen in their destructors? I.e., are we missing code here or do we just
have an undocumented lifetime ordering?



src/examples/test_csi_plugin.cpp
Lines 1165 (patched)
<https://reviews.apache.org/r/69787/#comment297778>

    Let's call out the type `unique_ptr<Server>` here.



src/examples/test_csi_plugin.cpp
Line 1025 (original), 1168 (patched)
<https://reviews.apache.org/r/69787/#comment297780>

    nit: Not this patch, but this is the default and not required in C++.


- Benjamin Bannier


On Jan. 18, 2019, 6:46 a.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69787/
> -----------------------------------------------------------
> 
> (Updated Jan. 18, 2019, 6:46 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-9517
>     https://issues.apache.org/jira/browse/MESOS-9517
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> If the `--proxy` flag is set, the test CSI plugin would forward all gRPC
> requests to the specified gRPC server URI, and return the response to
> the caller. This can be used to forward all CSI calls to a mock CSI
> plugin object in the test process.
> 
> 
> Diffs
> -----
> 
>   src/examples/test_csi_plugin.cpp af183037b280bab65578a4c447196a9ccf261e32 
> 
> 
> Diff: https://reviews.apache.org/r/69787/diff/1/
> 
> 
> Testing
> -------
> 
> Manually tweaked the plugin to forward requests to itself and all tests pass.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


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