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