> On April 18, 2018, 11:51 p.m., Chun-Hung Hsiao wrote: > > src/resource_provider/registrar.hpp > > Lines 67-73 (original), 69-75 (patched) > > <https://reviews.apache.org/r/66309/diff/3/?file=1995440#file1995440line69> > > > > Not yours, but if each `create()` corresponds to a different type of > > registrar, how about moving these functions into their corresponding > > `MasterRegistrar` and `AgentRegistrar`? > > > > I guess it depends on how meanful it is to encapsulate the actual type > > of registrar from the callers. > > Chun-Hung Hsiao wrote: > Also, if there is no need to encapsulate the actual type of registrar, > then it is meanleass to have these `create()` functions, since we don't do > extra checks and return errors.
I think having these factory functions still adds value as they hide away non-trivial details. The arguments pretty clearly communicate how they can be used, e.g., in the master we would not be able to transfer ownership of the underlying storage leaving only one possible factory to use. Let's keep this as is, dropping. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66309/#review201469 ----------------------------------------------------------- On April 19, 2018, 11:19 a.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66309/ > ----------------------------------------------------------- > > (Updated April 19, 2018, 11:19 a.m.) > > > Review request for mesos, Jie Yu and Jan Schlicht. > > > Bugs: MESOS-8735 > https://issues.apache.org/jira/browse/MESOS-8735 > > > Repository: mesos > > > Description > ------- > > This patch changes the way the storage backing an agent's resource > provider registrar is created: while before we created it implicitly > when constructing the registrar, we now consume storage passed on > construction. > > Being able to explicitly inject the used storage simplifies testing. > > > Diffs > ----- > > src/resource_provider/registrar.hpp > 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45 > src/resource_provider/registrar.cpp > 92ef9aecb1e93d10f46e53984391558f9901a60b > src/tests/resource_provider_manager_tests.cpp > c52541bf130ccf4795b989b53331176a64a097ea > > > Diff: https://reviews.apache.org/r/66309/diff/4/ > > > Testing > ------- > > `make check` > > > Thanks, > > Benjamin Bannier > >
