> On May 14, 2019, 12:04 p.m., Benjamin Bannier wrote: > > src/resource_provider/storage/provider.cpp > > Line 923 (original), 930 (patched) > > <https://reviews.apache.org/r/70620/diff/1/?file=2144328#file2144328line930> > > > > Let's create a dedicated error message. > > Chun-Hung Hsiao wrote: > Do you mean printing out an error or making it a hard failure? In theory > this should never fail and that's why I make it a hard assertion here.
I meant printing out some additional context around a possible error. Thinking about it again it might not be worth the hassle though. Dropping. > On May 14, 2019, 12:04 p.m., Benjamin Bannier wrote: > > src/resource_provider/storage/provider.cpp > > Lines 1067 (patched) > > <https://reviews.apache.org/r/70620/diff/1/?file=2144328#file2144328line1076> > > > > I don't think we need optional semantics here, I'd suggest to just work > > with an empty `Labels` here. > > > > If you want you can convert an empty `Labels` to a `None` when invoking > > `createRawDiskResource` below. I think even that function might not need > > optional semantics for labels though. > > Chun-Hung Hsiao wrote: > This is for maintaining the following proto2 <-> proto3 semantic, since > proto3 doesn't have an OPTIONAL semantic, instead any field w/ the default > value will not go onto the wire: > > Resource.DiskInfo.Source.has_metadata() <=> !VolumeInfo.context.empty() > > Since in practice there's no difference, we can change this to set up > `metadata`to an empty `Labels` even if the `context` map is empty, as long as > we handle backward compatibility carefully. But if we don't do this, it seems > more reasonable to me to pass in a `None` here than passing a `Labels` here > and doing a special check in another function. Makes sense, dropping. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70620/#review215220 ----------------------------------------------------------- On May 10, 2019, 3:14 a.m., Chun-Hung Hsiao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70620/ > ----------------------------------------------------------- > > (Updated May 10, 2019, 3:14 a.m.) > > > Review request for mesos, Benjamin Bannier and James DeFelice. > > > Bugs: MESOS-9395 > https://issues.apache.org/jira/browse/MESOS-9395 > > > Repository: mesos > > > Description > ------- > > To make SLRP more robust against non-conforming CSI plugins that change > volume contexts, the `getExistVolumes` method returns a list of resource > conversions consisting of one for converting old volume contexts to new > volume contexts, and one to remove missing volumes and add new volumes. > > To make the interfaces consistent, `getStoragePools` now also returns a > list of resource conversions consisting of one conversion. > > > Diffs > ----- > > include/mesos/mesos.proto dc6a87f14864cb4d46b9856f251c2946a475162d > include/mesos/v1/mesos.proto e8086e0f70cac73876a8ae31db3365b5059b5c44 > src/resource_provider/storage/provider.cpp > 999fe95bb6f38f5a25068accd854b37788b24028 > > > Diff: https://reviews.apache.org/r/70620/diff/1/ > > > Testing > ------- > > sudo make check > more testing done later in chain > > > Thanks, > > Chun-Hung Hsiao > >
