[
https://issues.apache.org/jira/browse/MESOS-3072?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14964922#comment-14964922
]
Alexander Rojas commented on MESOS-3072:
----------------------------------------
h3. Reviews
[r/38627/|https://reviews.apache.org/r/38627/]: Adds an overload of
ModuleManager::create() allowing overriding parameters programatically.
> Unify initialization of modularized components
> ----------------------------------------------
>
> Key: MESOS-3072
> URL: https://issues.apache.org/jira/browse/MESOS-3072
> Project: Mesos
> Issue Type: Improvement
> Components: modules
> Affects Versions: 0.22.0, 0.22.1, 0.23.0
> Reporter: Alexander Rojas
> Assignee: Alexander Rojas
> Labels: mesosphere
>
> h1.Introduction
> As it stands right now, default implementations of modularized components are
> required to have a non parametrized {{create()}} static method. This allows
> to write tests which can cover default implementations and modules based on
> these default implementations on a uniform way.
> For example, with the interface {{Foo}}:
> {code}
> class Foo {
> public:
> virtual ~Foo() {}
> virtual Future<int> hello() = 0;
> protected:
> Foo() {}
> };
> {code}
> With a default implementation:
> {code}
> class LocalFoo {
> public:
> Try<Foo*> create() {
> return new Foo;
> }
> virtual Future<int> hello() {
> return 1;
> }
> };
> {code}
> This allows to create typed tests which look as following:
> {code}
> typedef ::testing::Types<LocalFoo,
> tests::Module<Foo, TestLocalFoo>>
> FooTestTypes;
> TYPED_TEST_CASE(FooTest, FooTestTypes);
> TYPED_TEST(FooTest, ATest)
> {
> Try<Foo*> foo = TypeParam::create();
> ASSERT_SOME(foo);
> AWAIT_CHECK_EQUAL(foo.get()->hello(), 1);
> }
> {code}
> The test will be applied to each of types in the template parameters of
> {{FooTestTypes}}. This allows to test different implementation of an
> interface. In our code, it tests default implementations and a module which
> uses the same default implementation.
> The class {{tests::Module<typename T, ModuleID N>}} needs a little
> explanation, it is a wrapper around {{ModuleManager}} which allows the tests
> to encode information about the requested module in the type itself instead
> of passing a string to the factory method. The wrapper around create, the
> real important method looks as follows:
> {code}
> template<typename T, ModuleID N>
> static Try<T*> test::Module<T, N>::create()
> {
> Try<std::string> moduleName = getModuleName(N);
> if (moduleName.isError()) {
> return Error(moduleName.error());
> }
> return mesos::modules::ModuleManager::create<T>(moduleName.get());
> }
> {code}
> h1.The Problem
> Consider the following implementation of {{Foo}}:
> {code}
> class ParameterFoo {
> public:
> Try<Foo*> create(int i) {
> return new ParameterFoo(i);
> }
> ParameterFoo(int i) : i_(i) {}
> virtual Future<int> hello() {
> return i;
> }
> private:
> int i_;
> };
> {code}
> As it can be seen, this implementation cannot be used as a default
> implementation since its create API does not match the one of
> {{test::Module<>}}: {{create()}} has a different signature for both types. It
> is still a common situation to require initialization parameters for objects,
> however this constraint (keeping both interfaces alike) forces default
> implementations of modularized components to have default constructors,
> therefore the tests are forcing the design of the interfaces.
> Implementations which are supposed to be used as modules only, i.e. non
> default implementations are allowed to have constructor parameters, since the
> actual signature of their factory method is, this factory method's function
> is to decode the parameters and call the appropriate constructor:
> {code}
> template<typename T>
> T* Module<T>::create(const Parameters& params);
> {code}
> where parameters is just an array of key-value string pairs whose
> interpretation is left to the specific module. Sadly, this call is wrapped by
> {{ModuleManager}} which only allows module parameters to be passed from the
> command line and does not offer a programmatic way to feed construction
> parameters to modules.
> h1.The Ugly Workaround
> With the requirement of a default constructor and parameters devoid
> {{create()}} factory function, a common pattern (see
> [Authenticator|https://github.com/apache/mesos/blob/9d4ac11ed757aa5869da440dfe5343a61b07199a/include/mesos/authentication/authenticator.hpp])
> has been introduced to feed construction parameters into default
> implementation, this leads to adding an {{initialize()}} call to the public
> interface, which will have {{Foo}} become:
> {code}
> class Foo {
> public:
> virtual ~Foo() {}
> virtual Try<Nothing> initialize(Option<int> i) = 0;
> virtual Future<int> hello() = 0;
> protected:
> Foo() {}
> };
> {code}
> {{ParameterFoo}} will thus look as follows:
> {code}
> class ParameterFoo {
> public:
> Try<Foo*> create() {
> return new ParameterFoo;
> }
> ParameterFoo() : i_(None()) {}
> virtual Try<Nothing> initialize(Option<int> i) {
> if (i.isNone()) {
> return Error("Need value to initialize");
> }
> i_ = i;
> return Nothing;
> }
> virtual Future<int> hello() {
> if (i_.isNone()) {
> return Future<int>::failure("Not initialized");
> }
> return i_.get();
> }
> private:
> Option<int> i_;
> };
> {code}
> Look that this {{initialize()}} method now has to be implemented by all
> descendants of {{Foo}}, even if there's a {{DatabaseFoo}} which takes is
> return value for {{hello()}} from a DB, it will need to support {{int}} as an
> initialization parameter.
> The problem is more severe the more specific the parameter to
> {{initialize()}} is. For example, if there is a very complex structure
> implementing ACLs, all implementations of an authorizer will need to import
> this structure even if they can completely ignore it.
> In the {{Foo}} example if {{ParameterFoo}} were to become the default
> implementation of {{Foo}}, the tests would look as follows:
> {code}
> typedef ::testing::Types<ParameterFoo,
> tests::Module<Foo, TestParameterFoo>>
> FooTestTypes;
> TYPED_TEST_CASE(FooTest, FooTestTypes);
> TYPED_TEST(FooTest, ATest)
> {
> Try<Foo*> foo = TypeParam::create();
> ASSERT_SOME(foo);
> int fooValue = 1;
> foo.get()->initialize(fooValue);
> AWAIT_CHECK_EQUAL(foo.get()->hello(), fooValue);
> }
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
