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...@mesosphere.io> on 2017/10/27 23:59:58 UTC
Review Request 63385: Added utility functions for CSI responses and
volume ID and metadata.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63385/
-----------------------------------------------------------
Review request for mesos and Jie Yu.
Repository: mesos
Description
-------
Added utility functions for CSI responses and volume ID and metadata.
Diffs
-----
src/csi/utils.hpp PRE-CREATION
src/csi/utils.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/63385/diff/1/
Testing
-------
make
Thanks,
Chun-Hung Hsiao
Re: Review Request 63385: Added utility functions for CSI Plugin info
and volume attributes.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
> On Nov. 16, 2017, 12:33 a.m., Joseph Wu wrote:
> > src/csi/utils.cpp
> > Lines 41-46 (patched)
> > <https://reviews.apache.org/r/63385/diff/1/?file=1870949#file1870949line41>
> >
> > This comparison _looks_ wrong :)
> >
> > You should note that the object is empty, so they must be equal.
Removed since it is no longer needed.
> On Nov. 16, 2017, 12:33 a.m., Joseph Wu wrote:
> > src/csi/utils.cpp
> > Lines 62-71 (patched)
> > <https://reviews.apache.org/r/63385/diff/1/?file=1870949#file1870949line62>
> >
> > Does this version of the proto have the `access_mode` field?
> >
> > https://github.com/container-storage-interface/spec/blob/2561947ce5899bcb80e5dbaecb99afdce66e91d4/csi.proto#L228
> >
> > If not, consider adding a TODO?
> >
> > ---
> >
> > Also, it might be a good idea to add a note about the Proto3 `oneof` notation which enforces the existence of either `block` or `mount`. I'm guessing most contributors reading through this code might be more familiar with Proto2 rules.
Dropping this because this is no longer needed.
> On Nov. 16, 2017, 12:33 a.m., Joseph Wu wrote:
> > src/csi/utils.cpp
> > Lines 125-127 (patched)
> > <https://reviews.apache.org/r/63385/diff/1/?file=1870949#file1870949line125>
> >
> > Probably want to leave a TODO here, since `VolumeMetaData` no longer exists:
> > https://github.com/container-storage-interface/spec/commit/d2bdb9177e192ecee994c549f1b166fc073ff40b
Added a typedef for `google::protobuf::Map<string, string>` to `VolumeAttributes` and changed the conversion functions accordingly.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63385/#review191126
-----------------------------------------------------------
On Nov. 22, 2017, 5:21 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63385/
> -----------------------------------------------------------
>
> (Updated Nov. 22, 2017, 5:21 a.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added utility functions for CSI Plugin info and volume attributes.
>
>
> Diffs
> -----
>
> src/csi/utils.hpp PRE-CREATION
> src/csi/utils.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/63385/diff/2/
>
>
> Testing
> -------
>
> make
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 63385: Added utility functions for CSI responses
and volume ID and metadata.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63385/#review191126
-----------------------------------------------------------
src/csi/utils.cpp
Lines 41-46 (patched)
<https://reviews.apache.org/r/63385/#comment268736>
This comparison _looks_ wrong :)
You should note that the object is empty, so they must be equal.
src/csi/utils.cpp
Lines 62-71 (patched)
<https://reviews.apache.org/r/63385/#comment268733>
Does this version of the proto have the `access_mode` field?
https://github.com/container-storage-interface/spec/blob/2561947ce5899bcb80e5dbaecb99afdce66e91d4/csi.proto#L228
If not, consider adding a TODO?
---
Also, it might be a good idea to add a note about the Proto3 `oneof` notation which enforces the existence of either `block` or `mount`. I'm guessing most contributors reading through this code might be more familiar with Proto2 rules.
src/csi/utils.cpp
Lines 125-127 (patched)
<https://reviews.apache.org/r/63385/#comment268737>
Probably want to leave a TODO here, since `VolumeMetaData` no longer exists:
https://github.com/container-storage-interface/spec/commit/d2bdb9177e192ecee994c549f1b166fc073ff40b
- Joseph Wu
On Oct. 27, 2017, 4:59 p.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63385/
> -----------------------------------------------------------
>
> (Updated Oct. 27, 2017, 4:59 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added utility functions for CSI responses and volume ID and metadata.
>
>
> Diffs
> -----
>
> src/csi/utils.hpp PRE-CREATION
> src/csi/utils.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/63385/diff/1/
>
>
> Testing
> -------
>
> make
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 63385: Added utility functions for volume
attributes and printing CSI messages.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63385/
-----------------------------------------------------------
(Updated Nov. 28, 2017, 12:57 a.m.)
Review request for mesos and Jie Yu.
Changes
-------
Removed comparison for `GetPluginInfoResponse`, and made a generic output operator for all messages defined in namespace `::csi`.
Summary (updated)
-----------------
Added utility functions for volume attributes and printing CSI messages.
Repository: mesos
Description (updated)
-------
Added utility functions for volume attributes and printing CSI messages.
Diffs (updated)
-----
src/common/protobuf_utils.hpp 6f991e86e46512d5a2bc4e67e160189fccb77f6a
src/common/protobuf_utils.cpp c0ff306ae6c16cbba6fd08469b639b9f906c672b
src/csi/utils.hpp PRE-CREATION
src/csi/utils.cpp PRE-CREATION
src/tests/protobuf_utils_tests.cpp 2be26a5f467be85ec0ad28dcd8202e603e4a9d5e
Diff: https://reviews.apache.org/r/63385/diff/4/
Changes: https://reviews.apache.org/r/63385/diff/3-4/
Testing
-------
make check
Thanks,
Chun-Hung Hsiao
Re: Review Request 63385: Added utility functions for CSI Plugin info
and volume attributes.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63385/
-----------------------------------------------------------
(Updated Nov. 26, 2017, 11:10 p.m.)
Review request for mesos and Jie Yu.
Changes
-------
Addressed jieyu's comments and added a test.
Repository: mesos
Description
-------
Added utility functions for CSI Plugin info and volume attributes.
Diffs (updated)
-----
src/common/protobuf_utils.hpp eff106f0a546f459030d5cf5b42e5eb8d4d9a34c
src/common/protobuf_utils.cpp a5fc960b8387342eba4c66a711fedc92dd014e6f
src/csi/utils.hpp PRE-CREATION
src/csi/utils.cpp PRE-CREATION
src/tests/protobuf_utils_tests.cpp 2be26a5f467be85ec0ad28dcd8202e603e4a9d5e
Diff: https://reviews.apache.org/r/63385/diff/3/
Changes: https://reviews.apache.org/r/63385/diff/2-3/
Testing
-------
make
Thanks,
Chun-Hung Hsiao
Re: Review Request 63385: Added utility functions for CSI Plugin info
and volume attributes.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
> On Nov. 24, 2017, 11:49 p.m., Jie Yu wrote:
> > src/csi/utils.hpp
> > Lines 36-43 (patched)
> > <https://reviews.apache.org/r/63385/diff/2/?file=1899465#file1899465line36>
> >
> > Can you explain to me why you need the equality check for GetPluginInfoResponse?
For checking if the controller plugin component and the node plugin component are consistent.
> On Nov. 24, 2017, 11:49 p.m., Jie Yu wrote:
> > src/csi/utils.hpp
> > Lines 99 (patched)
> > <https://reviews.apache.org/r/63385/diff/2/?file=1899465#file1899465line99>
> >
> > We don't do typedef, especially in header.
> >
> > Also, I feel this is a general protobuf util helper. We should put it under src/common/protobuf_utils.hpp|cpp
> >
> > ```
> > convertMapsToLabels(...)
> > convertLabelsToMaps(...)
> > ```
I originally put the functions here because it's used for converting a proto3 string-to-string map field to our proto2 `Labels`. But there should be no difference for `google::protobuf::Map` to handle proto2 and proto3 maps, and thus it should be safe to use these functions to handle both proto2 and proto3 maps.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63385/#review191845
-----------------------------------------------------------
On Nov. 22, 2017, 5:21 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63385/
> -----------------------------------------------------------
>
> (Updated Nov. 22, 2017, 5:21 a.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added utility functions for CSI Plugin info and volume attributes.
>
>
> Diffs
> -----
>
> src/csi/utils.hpp PRE-CREATION
> src/csi/utils.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/63385/diff/2/
>
>
> Testing
> -------
>
> make
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 63385: Added utility functions for CSI Plugin info
and volume attributes.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63385/#review191845
-----------------------------------------------------------
src/csi/utils.hpp
Lines 36-43 (patched)
<https://reviews.apache.org/r/63385/#comment269781>
Can you explain to me why you need the equality check for GetPluginInfoResponse?
src/csi/utils.hpp
Lines 99 (patched)
<https://reviews.apache.org/r/63385/#comment269782>
We don't do typedef, especially in header.
Also, I feel this is a general protobuf util helper. We should put it under src/common/protobuf_utils.hpp|cpp
```
convertMapsToLabels(...)
convertLabelsToMaps(...)
```
- Jie Yu
On Nov. 22, 2017, 5:21 a.m., Chun-Hung Hsiao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63385/
> -----------------------------------------------------------
>
> (Updated Nov. 22, 2017, 5:21 a.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added utility functions for CSI Plugin info and volume attributes.
>
>
> Diffs
> -----
>
> src/csi/utils.hpp PRE-CREATION
> src/csi/utils.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/63385/diff/2/
>
>
> Testing
> -------
>
> make
>
>
> Thanks,
>
> Chun-Hung Hsiao
>
>
Re: Review Request 63385: Added utility functions for CSI Plugin info
and volume attributes.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63385/
-----------------------------------------------------------
(Updated Nov. 22, 2017, 5:21 a.m.)
Review request for mesos and Jie Yu.
Changes
-------
Removed comparisons between volumes since we no longer need that.
Summary (updated)
-----------------
Added utility functions for CSI Plugin info and volume attributes.
Repository: mesos
Description (updated)
-------
Added utility functions for CSI Plugin info and volume attributes.
Diffs (updated)
-----
src/csi/utils.hpp PRE-CREATION
src/csi/utils.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/63385/diff/2/
Changes: https://reviews.apache.org/r/63385/diff/1-2/
Testing
-------
make
Thanks,
Chun-Hung Hsiao