mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chun-Hung Hsiao <chhs...@apache.org>
Subject Re: Review Request 70247: Added an "unversioned" `VolumeCapability`.
Date Wed, 20 Mar 2019 17:36:09 GMT


> On March 20, 2019, 2:09 p.m., Benjamin Bannier wrote:
> > include/mesos/csi/compat.proto
> > Lines 1 (patched)
> > <https://reviews.apache.org/r/70247/diff/1/?file=2132974#file2132974line1>
> >
> >     I don't think these definitions need to be public as they never leave the agent.
Let's just add them to `src/csi/` or sim?

The definition will be used in the disk profile adaptor. This can be discussed, but I personally
feel that it's better if our module interface doesn't depend on a third-party protobuf so
we can control our own backward compatibility.


> On March 20, 2019, 2:09 p.m., Benjamin Bannier wrote:
> > include/mesos/csi/compat.proto
> > Lines 19 (patched)
> > <https://reviews.apache.org/r/70247/diff/1/?file=2132974#file2132974line19>
> >
> >     Looking at https://reviews.apache.org/r/70248/ I see that the `VolumeCapability`
we'd actually want to use most of the time would always come from `compat`.
> >     
> >     Can we define it e.g., in `mesos.csi` or alternatively `mesos.csi.types` instead?
That would seem more natural to me.

Yeah I can rename it to types. Currently the `mesos.csi` namespace import `.csi.v0` to make
it convenient for coding. Moving forwards, we can do either
1. make `mesos.csi.v0` import `.csi.v0` and `mesos.csi.v1` import `.csi.v1`. Thats the reason
I'm using `mesos.csi.compat`, but `mesos.csi.types` also works. (Actually `types` is one of
the name I was considering ;) )
2. Don't do the import. As as result, all CSI v0 protobufs would be like `::csi::v0::XXXMessage`
in the codebase, which is probably fine, just look a bit tedious.

What's your opinion on this?


> On March 20, 2019, 2:09 p.m., Benjamin Bannier wrote:
> > include/mesos/csi/compat.proto
> > Lines 27-30 (patched)
> > <https://reviews.apache.org/r/70247/diff/1/?file=2132974#file2132974line27>
> >
> >     This promises a pretty hard thing. Why are we so sure this is always possible?
> >     
> >     This would be less concerning if this file would contain just types never leaving
a single node (i.e., no concerns about mixing different proto versions).
> >     
> >     Let's promise that we'll try to faithfully reconstruct types instead?

Again, this is for using it in the public module interface.

I guess I should have probably loosen the constraint about directly parsing it from a versioned
CSI protobuf JSON. We should just ensure that this protobuf can hold all information for different
versioned CSI protobuf without loosing the information, and let the profile adaptor module
to do the convertios from a versioned CSI protobuf to this one.


> On March 20, 2019, 2:09 p.m., Benjamin Bannier wrote:
> > src/csi/compat.cpp
> > Lines 25-61 (patched)
> > <https://reviews.apache.org/r/70247/diff/1/?file=2132977#file2132977line25>
> >
> >     `return google::protobuf::util::MessageDifferencer::Equals(left, right)`?

The reason we generally don't use `MessageDifferencer` is that we probably want to define
our own equality semantics, for example, the "ordering may not matter" constraint here (although
it's not implemented). Also, do we want to compare unknown fields?

If we want a complete message equality check, then yes I'll just use `MessageDifferencer`.


- Chun-Hung


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


On March 20, 2019, 6:15 a.m., Chun-Hung Hsiao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70247/
> -----------------------------------------------------------
> 
> (Updated March 20, 2019, 6:15 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-9625
>     https://issues.apache.org/jira/browse/MESOS-9625
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> To support both CSI v0 and v1, the "unversioned" `VolumeCapability` is
> designed to satisfy the following compatibility guarantees:
> 
> 1. It can be parsed from the JSON representation of a CSI v0 or v1
>    `VolumeCapability`. This is for making the `UriDiskProfileAdaptor`
>    backward compatible, and the profile service can simply provide
>    `VolumeCapability` of a specific CSI version without rework.
> 
> 2. The unversioned `VolumeCapability` parsed from a versioned one should
>    be able to used to reconstruct the original versioned
>    `VolumeCapability`, and can be upgraded/downgraded to a different
>    CSI version and preserve as much semantics as possible.
> 
> 3. If an backward-incompatible change is introduced in future CSI
>    `VolumeCapability`, the unversioned `VolumeCapability` can provide a
>    way to do a backward compatible upgrade.
> 
> 
> Diffs
> -----
> 
>   include/mesos/csi/compat.hpp PRE-CREATION 
>   include/mesos/csi/compat.proto PRE-CREATION 
>   src/CMakeLists.txt 3397c3b1d4e8a7900b2e5f870679cc7aa30b4be2 
>   src/Makefile.am d451d7cabe3bf5d4f5039cfac5de1b03ef891d07 
>   src/csi/compat.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70247/diff/1/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>


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