You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Chun-Hung Hsiao <ch...@apache.org> on 2019/01/18 05:46:02 UTC
Review Request 69787: Added a proxy mode to the test CSI plugin.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69787/
-----------------------------------------------------------
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
Re: Review Request 69787: Added a proxy mode to the test CSI plugin.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69787/#review212131
-----------------------------------------------------------
PASS: Mesos patch 69787 was successfully built and tested.
Reviews applied: `['69787']`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2799/mesos-review-69787
- Mesos Reviewbot Windows
On Jan. 18, 2019, 5: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, 5: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
>
>
Re: Review Request 69787: Added a forwarding mode to the test CSI
plugin.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Jan. 18, 2019, 12:03 p.m., Benjamin Bannier wrote:
> > src/examples/test_csi_plugin.cpp
> > Line 1025 (original), 1168 (patched)
> > <https://reviews.apache.org/r/69787/diff/1/?file=2120294#file2120294line1187>
> >
> > nit: Not this patch, but this is the default and not required in C++.
>
> Chun-Hung Hsiao wrote:
> Could you explain more? Do you mean I should do `return 0` instead?
You could just remove this line, https://en.cppreference.com/w/cpp/language/return#Notes.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69787/#review212136
-----------------------------------------------------------
On Jan. 25, 2019, 1:35 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69787/
> -----------------------------------------------------------
>
> (Updated Jan. 25, 2019, 1:35 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
>
>
> Bugs: MESOS-9517
> https://issues.apache.org/jira/browse/MESOS-9517
>
>
> Repository: mesos
>
>
> Description
> -------
>
> If the `--forward` 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/3/
>
>
> Testing
> -------
>
> Manually tweaked the plugin to forward requests to itself and all tests pass.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 69787: Added a proxy mode to the test CSI plugin.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On Jan. 18, 2019, 11:03 a.m., Benjamin Bannier wrote:
> > src/examples/test_csi_plugin.cpp
> > Lines 930-931 (patched)
> > <https://reviews.apache.org/r/69787/diff/1/?file=2120294#file2120294line949>
> >
> > Just accept values here? Move construction of `unique_ptr` is cheap and possibly elided.
Created the service and the completion queue in `CSIProxy` for proper shutdown instead so this is no longer an issue.
> On Jan. 18, 2019, 11:03 a.m., Benjamin Bannier wrote:
> > src/examples/test_csi_plugin.cpp
> > Lines 978 (patched)
> > <https://reviews.apache.org/r/69787/diff/1/?file=2120294#file2120294line997>
> >
> > 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).
As discussed, this would lead to an undefined behavior in C++ because of the undetermined evaluation order for function arguments. Dropping this. Instead, I'll add more comments to explain the ownership transition.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69787/#review212136
-----------------------------------------------------------
On Jan. 23, 2019, 7:14 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69787/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2019, 7:14 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
>
>
> 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/2/
>
>
> Testing
> -------
>
> Manually tweaked the plugin to forward requests to itself and all tests pass.
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 69787: Added a proxy mode to the test CSI plugin.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On Jan. 18, 2019, 11:03 a.m., Benjamin Bannier wrote:
> > src/examples/test_csi_plugin.cpp
> > Lines 970 (patched)
> > <https://reviews.apache.org/r/69787/diff/1/?file=2120294#file2120294line989>
> >
> > Could we give this a better name, e.g., `completionQueue`? That would be less gRPC'y, but more Mesos'y.
Done.
> On Jan. 18, 2019, 11:03 a.m., Benjamin Bannier wrote:
> > src/examples/test_csi_plugin.cpp
> > Lines 990 (patched)
> > <https://reviews.apache.org/r/69787/diff/1/?file=2120294#file2120294line1009>
> >
> > We should log something here as this could fail in general.
Properly handled `ok == false` in each scenario.
> On Jan. 18, 2019, 11:03 a.m., Benjamin Bannier wrote:
> > src/examples/test_csi_plugin.cpp
> > Line 1020 (original), 1146 (patched)
> > <https://reviews.apache.org/r/69787/diff/1/?file=2120294#file2120294line1165>
> >
> > 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?
Added the proper shutdown logic. Will add more comments.
> On Jan. 18, 2019, 11:03 a.m., Benjamin Bannier wrote:
> > src/examples/test_csi_plugin.cpp
> > Line 1025 (original), 1168 (patched)
> > <https://reviews.apache.org/r/69787/diff/1/?file=2120294#file2120294line1187>
> >
> > nit: Not this patch, but this is the default and not required in C++.
Could you explain more? Do you mean I should do `return 0` instead?
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69787/#review212136
-----------------------------------------------------------
On Jan. 18, 2019, 5: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, 5: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
>
>
Re: Review Request 69787: Added a proxy mode to the test CSI plugin.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
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
>
>
Re: Review Request 69787: Added a proxy mode to the test CSI plugin.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69787/
-----------------------------------------------------------
(Updated Jan. 23, 2019, 7:13 a.m.)
Review request for mesos, Benjamin Bannier and Jie Yu.
Changes
-------
Addressed some of Benjamin's comments. Will fix the remaining.
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 (updated)
-----
src/examples/test_csi_plugin.cpp af183037b280bab65578a4c447196a9ccf261e32
Diff: https://reviews.apache.org/r/69787/diff/2/
Changes: https://reviews.apache.org/r/69787/diff/1-2/
Testing
-------
Manually tweaked the plugin to forward requests to itself and all tests pass.
Thanks,
Chun-Hung Hsiao