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/05/10 01:17:09 UTC
Review Request 70621: Used full paths as volume IDs for the test CSI
plugin.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70621/
-----------------------------------------------------------
Review request for mesos and Benjamin Bannier.
Bugs: MESOS-9395
https://issues.apache.org/jira/browse/MESOS-9395
Repository: mesos
Description
-------
The full paths of simulated volumes are now in their ID instead of their
contextual information. This simplifies SLRP tests, and makes it cleaner
if we want to customize the contextual information in the future.
Diffs
-----
src/examples/test_csi_plugin.cpp 03f782ead136a79c4c5b099496072933f6737598
src/tests/storage_local_resource_provider_tests.cpp ec2222d7aeef1cb3d1a5b4b3419dfd912d41a8c6
Diff: https://reviews.apache.org/r/70621/diff/1/
Testing
-------
make check
Thanks,
Chun-Hung Hsiao
Re: Review Request 70621: Used full paths as volume IDs for the test
CSI plugin.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70621/#review215495
-----------------------------------------------------------
Ship it!
LGTM!
- Benjamin Bannier
On May 22, 2019, 5:03 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70621/
> -----------------------------------------------------------
>
> (Updated May 22, 2019, 5:03 a.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-9395
> https://issues.apache.org/jira/browse/MESOS-9395
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The full paths of simulated volumes are now in their ID instead of their
> contextual information. This simplifies SLRP tests, and makes it cleaner
> if we want to customize the contextual information in the future.
>
>
> Diffs
> -----
>
> src/examples/test_csi_plugin.cpp 03f782ead136a79c4c5b099496072933f6737598
> src/tests/storage_local_resource_provider_tests.cpp ec2222d7aeef1cb3d1a5b4b3419dfd912d41a8c6
>
>
> Diff: https://reviews.apache.org/r/70621/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 70621: Used full paths as volume IDs for 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/70621/
-----------------------------------------------------------
(Updated May 22, 2019, 3:03 a.m.)
Review request for mesos and Benjamin Bannier.
Changes
-------
Addressed Benjamin's comments.
Bugs: MESOS-9395
https://issues.apache.org/jira/browse/MESOS-9395
Repository: mesos
Description
-------
The full paths of simulated volumes are now in their ID instead of their
contextual information. This simplifies SLRP tests, and makes it cleaner
if we want to customize the contextual information in the future.
Diffs (updated)
-----
src/examples/test_csi_plugin.cpp 03f782ead136a79c4c5b099496072933f6737598
src/tests/storage_local_resource_provider_tests.cpp ec2222d7aeef1cb3d1a5b4b3419dfd912d41a8c6
Diff: https://reviews.apache.org/r/70621/diff/2/
Changes: https://reviews.apache.org/r/70621/diff/1-2/
Testing
-------
make check
Thanks,
Chun-Hung Hsiao
Re: Review Request 70621: Used full paths as volume IDs for the test
CSI plugin.
Posted by Benjamin Bannier <bb...@apache.org>.
> On May 14, 2019, 1:37 p.m., Benjamin Bannier wrote:
> > src/examples/test_csi_plugin.cpp
> > Lines 1250 (patched)
> > <https://reviews.apache.org/r/70621/diff/1/?file=2144329#file2144329line1274>
> >
> > Can we do that "normalization" when we initialize `workDir` instead?
>
> Chun-Hung Hsiao wrote:
> Doing that seems to increase the coupling between the initialization of `workDir`, which happens in the constructor, and the code here. Dropping.
Fair enough, but I could respond that doing that normalization in the ctr would help to enforce internal invariants.
> On May 14, 2019, 1:37 p.m., Benjamin Bannier wrote:
> > src/examples/test_csi_plugin.cpp
> > Line 1269 (original), 1269 (patched)
> > <https://reviews.apache.org/r/70621/diff/1/?file=2144329#file2144329line1296>
> >
> > We are leaving `id` default-initialized here.
>
> Chun-Hung Hsiao wrote:
> No it's the context which got default-initialized. Let me make it explicit here.
LGTM. The alternative would have been to e.g., add a ctr for `VolumeInfo`.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70621/#review215248
-----------------------------------------------------------
On May 22, 2019, 5:03 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70621/
> -----------------------------------------------------------
>
> (Updated May 22, 2019, 5:03 a.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-9395
> https://issues.apache.org/jira/browse/MESOS-9395
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The full paths of simulated volumes are now in their ID instead of their
> contextual information. This simplifies SLRP tests, and makes it cleaner
> if we want to customize the contextual information in the future.
>
>
> Diffs
> -----
>
> src/examples/test_csi_plugin.cpp 03f782ead136a79c4c5b099496072933f6737598
> src/tests/storage_local_resource_provider_tests.cpp ec2222d7aeef1cb3d1a5b4b3419dfd912d41a8c6
>
>
> Diff: https://reviews.apache.org/r/70621/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 70621: Used full paths as volume IDs for the test
CSI plugin.
Posted by Chun-Hung Hsiao <ch...@apache.org>.
> On May 14, 2019, 11:37 a.m., Benjamin Bannier wrote:
> > src/examples/test_csi_plugin.cpp
> > Line 1253 (original), 1244 (patched)
> > <https://reviews.apache.org/r/70621/diff/1/?file=2144329#file2144329line1267>
> >
> > It would be great to have some testing that `getVolumePath -> parseVolumePath` roundtrips. Let's either add a simple dedicated test or, since this is test code, just add an assertion in e.g., `getVolumePath` that the created path would roundtrip.
Sure will do. However this function does not decode volume names so extra work needs to be done.
> On May 14, 2019, 11:37 a.m., Benjamin Bannier wrote:
> > src/examples/test_csi_plugin.cpp
> > Lines 1250 (patched)
> > <https://reviews.apache.org/r/70621/diff/1/?file=2144329#file2144329line1274>
> >
> > Can we do that "normalization" when we initialize `workDir` instead?
Doing that seems to increase the coupling between the initialization of `workDir`, which happens in the constructor, and the code here. Dropping.
> On May 14, 2019, 11:37 a.m., Benjamin Bannier wrote:
> > src/examples/test_csi_plugin.cpp
> > Line 1269 (original), 1269 (patched)
> > <https://reviews.apache.org/r/70621/diff/1/?file=2144329#file2144329line1296>
> >
> > We are leaving `id` default-initialized here.
No it's the context which got default-initialized. Let me make it explicit here.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70621/#review215248
-----------------------------------------------------------
On May 10, 2019, 1:17 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70621/
> -----------------------------------------------------------
>
> (Updated May 10, 2019, 1:17 a.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-9395
> https://issues.apache.org/jira/browse/MESOS-9395
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The full paths of simulated volumes are now in their ID instead of their
> contextual information. This simplifies SLRP tests, and makes it cleaner
> if we want to customize the contextual information in the future.
>
>
> Diffs
> -----
>
> src/examples/test_csi_plugin.cpp 03f782ead136a79c4c5b099496072933f6737598
> src/tests/storage_local_resource_provider_tests.cpp ec2222d7aeef1cb3d1a5b4b3419dfd912d41a8c6
>
>
> Diff: https://reviews.apache.org/r/70621/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 70621: Used full paths as volume IDs for the test
CSI plugin.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70621/#review215248
-----------------------------------------------------------
src/examples/test_csi_plugin.cpp
Lines 215 (patched)
<https://reviews.apache.org/r/70621/#comment301878>
Let's add some additional context to this assertion.
src/examples/test_csi_plugin.cpp
Line 226 (original), 223 (patched)
<https://reviews.apache.org/r/70621/#comment301879>
Ditto.
src/examples/test_csi_plugin.cpp
Lines 228 (patched)
<https://reviews.apache.org/r/70621/#comment301880>
Ditto.
src/examples/test_csi_plugin.cpp
Line 1253 (original), 1244 (patched)
<https://reviews.apache.org/r/70621/#comment301881>
It would be great to have some testing that `getVolumePath -> parseVolumePath` roundtrips. Let's either add a simple dedicated test or, since this is test code, just add an assertion in e.g., `getVolumePath` that the created path would roundtrip.
src/examples/test_csi_plugin.cpp
Lines 1250 (patched)
<https://reviews.apache.org/r/70621/#comment301882>
Can we do that "normalization" when we initialize `workDir` instead?
src/examples/test_csi_plugin.cpp
Lines 1257-1261 (original), 1259-1264 (patched)
<https://reviews.apache.org/r/70621/#comment301883>
Not added here, but I feel `strings::tokenize` would have made this more readible, e.g.,
````
vector<string> components = strings::tokenize(basename, "-", 2);
if (components.size() != 2) {
return Error("Cannot find delimiter '-' in " + basename); // FYI: Made this error more explicit.
}
Try<Bytes> bytes = Bytes::parse(components[1]);
````
src/examples/test_csi_plugin.cpp
Line 1269 (original), 1269 (patched)
<https://reviews.apache.org/r/70621/#comment301884>
We are leaving `id` default-initialized here.
- Benjamin Bannier
On May 10, 2019, 3:17 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70621/
> -----------------------------------------------------------
>
> (Updated May 10, 2019, 3:17 a.m.)
>
>
> Review request for mesos and Benjamin Bannier.
>
>
> Bugs: MESOS-9395
> https://issues.apache.org/jira/browse/MESOS-9395
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The full paths of simulated volumes are now in their ID instead of their
> contextual information. This simplifies SLRP tests, and makes it cleaner
> if we want to customize the contextual information in the future.
>
>
> Diffs
> -----
>
> src/examples/test_csi_plugin.cpp 03f782ead136a79c4c5b099496072933f6737598
> src/tests/storage_local_resource_provider_tests.cpp ec2222d7aeef1cb3d1a5b4b3419dfd912d41a8c6
>
>
> Diff: https://reviews.apache.org/r/70621/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>