> 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
>
>
|