> On March 19, 2019, 1:12 p.m., Benjamin Bannier wrote: > > src/resource_provider/storage/provider.cpp > > Line 3734 (original), 3140 (patched) > > <https://reviews.apache.org/r/70169/diff/2/?file=2131596#file2131596line3745> > > > > Preexisting, but isn't this already covered by above > > `operations_dropped` counter for `UNKNOWN`?
No. The previous `switch` trick would skip `UNKNOWN`, as we don't want to create `operations/unknown/pending`, `operations/unknown/finished` and `operations/unknown/failed`. Dropping. I'm planning to do a small cleanup by, e.g., introducing `isSupportedOperation`, and replace the `switch` trick with a loop that uses the helpers provided by protobuf. > On March 19, 2019, 1:12 p.m., Benjamin Bannier wrote: > > src/resource_provider/storage/provider_process.hpp > > Line 369 (original), 344 (patched) > > <https://reviews.apache.org/r/70169/diff/2/?file=2131597#file2131597line370> > > > > Preexisting condition, but should we make this class non-copyable? It already is as the copy ctor and assignment have been marked as deleted. Dropping. > On March 19, 2019, 1:12 p.m., Benjamin Bannier wrote: > > src/resource_provider/storage/provider_process.hpp > > Line 402 (original), 373 (patched) > > <https://reviews.apache.org/r/70169/diff/2/?file=2131597#file2131597line403> > > > > Composition instead of inheritance seems to work just fine here, let's > > do that instead. Yeah both work. The inheritance approach would enable us to use `metrics.csi_plugin_container_termination` instead of `metrics.csiPluginMetrics.container_termination`. WDYT? Also let's move the discussion to r/70245. - Chun-Hung ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70169/#review213801 ----------------------------------------------------------- On March 12, 2019, 8:43 p.m., Chun-Hung Hsiao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70169/ > ----------------------------------------------------------- > > (Updated March 12, 2019, 8:43 p.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 > ------- > > Refactored SLRP to use `ServiceManager`. > > > Diffs > ----- > > src/resource_provider/storage/provider.cpp > fea623c292158deb1b4b4b9ab1ac208031471519 > src/resource_provider/storage/provider_process.hpp > a5536b3d735e01eb1c4dc52d0602d973155f3c93 > > > Diff: https://reviews.apache.org/r/70169/diff/2/ > > > Testing > ------- > > sudo make check > > > Thanks, > > Chun-Hung Hsiao > >
