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