> On May 4, 2017, 6:29 p.m., Jie Yu wrote: > > src/slave/containerizer/containerizer.hpp > > Lines 66 (patched) > > <https://reviews.apache.org/r/58999/diff/1/?file=1708769#file1708769line66> > > > > Why optional? > > Kapil Arya wrote: > That way we don't have to update the existing unit tests involving > containerizers. > > Jie Yu wrote: > Can you try using default parameter? > > Jie Yu wrote: > Even default parameter is hacky. We should at least have a TODO > somewhere. I saw the isolator will simply call secretFetcher.get() assuming > it's Some(), while you're passing None() to containerizer in the test. > > I don't like the way we inject `Fetcher` in tests also. That's the reason > why RAW pointer is evil. If you have a managed pointer, you probably don't > have this issue. Maybe we should use a managed pointer in the interface? The > fact that the module create returns a raw pointer is a bad decision in > retrospect. It should have been unique_ptr or Owned pointer. > > Jie Yu wrote: > Any reason this cannot be a Shared pointer? `Shared<SecretFetcher>`? > > just make the method in SecretFetcher const. > > Kapil Arya wrote: > Are you suggesting `SecretFecther::fetch(...) const;` ? > > Kapil Arya wrote: > I am not sure how we can use managed pointers with modules because a > module might just return a pointer to a static object without ever calling > `new`. > > I think we'll need to use raw pointer with a default parameter instead. > > Jie Yu wrote: > As I said, the fact that module returns a raw pointer is a tech debt. We > should force it to return an Owned pointer or unique_ptr in the future. > > Can you follow up with patches to fix that if possible? I think we can > change the module interface at anytime? > > Jie Yu wrote: > For the time being, can you just add a documentation to the SecretFetcher > saying that the create must returns a dynamically allocated object whose > lifecycle should be delegate to the caller? > > Kapil Arya wrote: > Yes, I'll add a comment/TODO for now. I already spoke with Benjamin about > making appropriate changes to the module interface in 1.4 timeframe. The > module API is marked experimental and can change at any time.
Added comments to `mesos/module.hpp` (see https://reviews.apache.org/r/58759/diff/5#0.2). - Kapil ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58999/#review173969 ----------------------------------------------------------- On May 5, 2017, 6:41 a.m., Kapil Arya wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58999/ > ----------------------------------------------------------- > > (Updated May 5, 2017, 6:41 a.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone. > > > Repository: mesos > > > Description > ------- > > Updated Containerizer to accept SecretResolver. > > > Diffs > ----- > > src/local/local.cpp e47980929db2da1f31cf899a0e1fc452070e11f3 > src/slave/containerizer/containerizer.hpp > 4c31a1f5c853c1dc66480c7b4c867a87a1bb5c41 > src/slave/containerizer/containerizer.cpp > 9024371b6c4228f0903cfeef3bbec736e1a425f8 > src/slave/containerizer/mesos/containerizer.hpp > 29a99f33e646593127b9dc126f398f7bca88e21d > src/slave/containerizer/mesos/containerizer.cpp > b58baed64480e22f640a4852537f85922ed382ae > src/slave/flags.hpp c7a4604ed994e15c1db6accfaded2e882f1aec94 > src/slave/flags.cpp c50e43c0e0ba633f6b905b0d78668c0a0eebb173 > src/slave/main.cpp 72b141cb66f9df5bcc7b3f8cfcc2b06fcbd17e52 > src/tests/containerizer/docker_volume_isolator_tests.cpp > b47a6b5081a63ac474ac4634701b1a572eb58137 > src/tests/containerizer/mesos_containerizer_tests.cpp > 13e0f7e603a3ffdd0965b253d7abfe6a069cd2b4 > > > Diff: https://reviews.apache.org/r/58999/diff/2/ > > > Testing > ------- > > > Thanks, > > Kapil Arya > >
