> On March 19, 2019, 2:12 p.m., Benjamin Bannier wrote: > > src/csi/service_manager.cpp > > Lines 184-185 (patched) > > <https://reviews.apache.org/r/70168/diff/6/?file=2132460#file2132460line184> > > > > In Mesos we typically expand types unless they are already mentioned > > somewhere in the statement or are nested types (e.g., iterators). > > ```` > > foreach (const CSIPluginContainerInfo::Service& service, services) { > > foreach (const CSIPluginContainerInfo& container, info.containers()) { > > ... > > } > > } > > ```` > > Chun-Hung Hsiao wrote: > This is actually the debate around `auto` ;) But I don't have a strong > opinion on this and the types seems less likely to change, I'll take your > suggestion.
I do have a strong opinion, but I understand that consistency trumps everything ;) > On March 19, 2019, 2:12 p.m., Benjamin Bannier wrote: > > src/csi/service_manager.cpp > > Lines 316 (patched) > > <https://reviews.apache.org/r/70168/diff/6/?file=2132460#file2132460line316> > > > > Let's just use the `static` function like we had previously, > > ``` > > static ContainerID getContainerId( > > const CSIPluginInfo& info, > > const CSIPluginContainerInfo& container); > > ``` > > Chun-Hung Hsiao wrote: > We'll have to pass in `containerPrefix` as well. Do you have a reason > that makes you prefer a static function, given that this is already in the > implementation file so won't be leaked? I primarily suggested to use a free-standing function as before to reduce number of things we do in this patch. IMO free-standing functions have better defined state and coupling; it is much easier to accidentally completely entangle a member function with other, unrelated instance state than it is to do the same to a non-member function. > On March 19, 2019, 2:12 p.m., Benjamin Bannier wrote: > > src/csi/service_manager.cpp > > Lines 332-338 (patched) > > <https://reviews.apache.org/r/70168/diff/6/?file=2132460#file2132460line332> > > > > Maybe the following instead? > > ```` > > auto it = std::find_if( > > info.containers().begin(), > > info.containers().end(), > > [this, containerId](const CSIPluginContainerInfo& info) { > > return getContainerId(info) == containerId; > > }); > > > > return it != info.containers().end() > > ? *it > > : Option<CSIPluginContainerInfo>::none(); > > ```` > > Chun-Hung Hsiao wrote: > I actually thought about this before. But I'm not sure if this is more > readable than the current one. WDYT? My argument would be that the replacment would be idiomatic C++ and have a higher level of abstraction. That should in general not only reduce the likelihood of bugs, but also lead to more maintainable code. See e.g., https://sean-parent.stlab.cc/presentations/2013-09-11-cpp-seasoning/cpp-seasoning.pdf. IMO raising the abstraction level in Mesos for such issues would be helpful. We currently don't seem to have a strong preference in the project one way or another though, so it's a matter of taste which I'll leave up to you. > On March 19, 2019, 2:12 p.m., Benjamin Bannier wrote: > > src/csi/service_manager.cpp > > Lines 519-520 (patched) > > <https://reviews.apache.org/r/70168/diff/6/?file=2132460#file2132460line519> > > > > How about avoiding the hard assertion here and instead returning a > > `Failure`? > > > > Since this was in the existing code, so maybe let's do any cleanup in a > > separate patch. > > Chun-Hung Hsiao wrote: > This should not happen. The public `getServiceEndpoint` function already > returns a failure. Dropping. Sounds good. > On March 19, 2019, 2:12 p.m., Benjamin Bannier wrote: > > src/csi/service_manager.cpp > > Lines 614 (patched) > > <https://reviews.apache.org/r/70168/diff/6/?file=2132460#file2132460line614> > > > > Should we have a metric for the start action as well? > > Chun-Hung Hsiao wrote: > It seems not adding much value to me. The start action count itself > doesn't provide much value unless comparing it with the termination count, > which could be used to deduce the number of running containers. If that's the > purpose, then maybe it's more direct to just add a metrics for running > containers. > > However, the number of running containers is tightly coupled with whether > the RP would launch a single plugin container (unified CSI deployment) or two > containers (split-component deployment). And that means potentially the > monitoring configuration using this metric would be couple with the RP > configs, which seems not good. The termination count itself seems sufficient > to detect plugin failures. > > We can discuss if this is necessary in another ticket. Make sense. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70168/#review213774 ----------------------------------------------------------- On March 20, 2019, 6:36 a.m., Chun-Hung Hsiao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70168/ > ----------------------------------------------------------- > > (Updated March 20, 2019, 6:36 a.m.) > > > Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht. > > > Bugs: MESOS-9632 > https://issues.apache.org/jira/browse/MESOS-9632 > > > Repository: mesos > > > Description > ------- > > The `ServiceManager` will be used to refactor SLRP to move container > management out from SLRP. It is agnostic to CSI versions, so can be used > to manage plugin containers for both CSI v0 and v1 plugins. Most of its > logic are adapted from the SLRP code. > > > Diffs > ----- > > src/CMakeLists.txt 3397c3b1d4e8a7900b2e5f870679cc7aa30b4be2 > src/Makefile.am d451d7cabe3bf5d4f5039cfac5de1b03ef891d07 > src/csi/service_manager.hpp PRE-CREATION > src/csi/service_manager.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/70168/diff/7/ > > > Testing > ------- > > Testing done later in chain. > > > Thanks, > > Chun-Hung Hsiao > >
