> 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.
Are you suggesting `SecretFecther::fetch(...) const;` ? - Kapil ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58999/#review173969 ----------------------------------------------------------- On May 4, 2017, 4:07 p.m., Kapil Arya wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58999/ > ----------------------------------------------------------- > > (Updated May 4, 2017, 4:07 p.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone. > > > Repository: mesos > > > Description > ------- > > Updated Containerizer to accept SecretFetcher. > > > 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/cluster.hpp 65634121b7fe076a7cd9a1c5aa6101a96b1c009d > src/tests/cluster.cpp a4f57e0e9c4f221c42bdbb36d8925f688b3d742f > 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/1/ > > > Testing > ------- > > > Thanks, > > Kapil Arya > >
