[ https://issues.apache.org/jira/browse/MESOS-3248?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14680040#comment-14680040 ]
Alexander Rojas edited comment on MESOS-3248 at 8/10/15 12:39 PM: ------------------------------------------------------------------ My solution for this problem will look as follows: Under this proposal, the first step is to overload the factory method {{create()}} of the {{ModuleManager}} by adding the following: {code} template <typename T> static Try<T*> ModuleManager<T>::create( const std::string& moduleName, const Parameters& params) { synchronized (mutex) { if (!moduleBases.contains(moduleName)) { return Error( "Module '" + moduleName + "' unknown"); } Module<T>* module = (Module<T>*) moduleBases[moduleName]; if (module->create == NULL) { return Error( "Error creating module instance for '" + moduleName + "': " "create() method not found"); } std::string expectedKind = kind<T>(); if (expectedKind != module->kind) { return Error( "Error creating module instance for '" + moduleName + "': " "module is of kind '" + module->kind + "', but the requested " "kind is '" + expectedKind + "'"); } T* instance = module->create(params); if (instance == NULL) { return Error("Error creating Module instance for '" + moduleName + "'"); } return instance; } } {code} Then the original {{create()}} method can be simplified to: {code} template <typename T> static Try<T*> ModuleManager<T>::create(const std::string& moduleName) { return create(moduleName, moduleParameters[moduleName]); } {code} {{Foo}} remains unchanged: {code} class Foo { public: virtual ~Foo() {} virtual Future<int> hello() = 0; protected: Foo() {} }; {code} Wile {{ParameterFoo}} is kept simple with only one factory function extra: {code} class ParameterFoo { public: Try<Foo*> create(int i) { return new ParameterFoo(i); } Try<Foo*> create(const Parameters& params) { Option<int> param = None; std::size_t error; for (const auto& param : params.parameter()) { if (param.key() == "i") { param = std::stoi(param.value(), &error); if (error == 0) { return Error("Could not parse parameters"); } } } if (param.isNone()) { return Error("Wrong type given in the parameters"); } return create(param.get()); } ParameterFoo(int i) : i_(i) {} virtual Future<int> hello() { return i; } private: int i_; }; {code} Some changes in {{tests::Module}} will be needed, adding an overload for create: {code} template<typename T> static Try<T*> tests::Module<T>::create(const Parameters& params) { Try<std::string> moduleName = getModuleName(N); if (moduleName.isError()) { return Error(moduleName.error()); } return mesos::modules::ModuleManager::create<T>(moduleName.get(), params); } {code} The test can thus be written as: {code} typedef ::testing::Types<ParameterFoo, tests::Module<Foo, TestParameterFoo>> FooTestTypes; TYPED_TEST_CASE(FooTest, FooTestTypes); TYPED_TEST(FooTest, ATest) { int fooValue = 1; // This part can go in the fixture set up Parameters params; Parameter* param = params.add_parameter(); param->set_key("i"); param->set_value(std::to_string(fooValue)); Try<Foo*> foo = TypeParam::create(params); ASSERT_SOME(foo); AWAIT_CHECK_EQUAL(foo.get()->hello(), fooValue); } {code} was (Author: arojas): My solution for this problem will look as follows: Under this proposal, the first step is to overload the factory method {{create()}} of the {{ModuleManager}} by adding the following: {code} template <typename T> static Try<T*> ModuleManager<T>::create( const std::string& moduleName, const Parameters& params) { synchronized (mutex) { if (!moduleBases.contains(moduleName)) { return Error( "Module '" + moduleName + "' unknown"); } Module<T>* module = (Module<T>*) moduleBases[moduleName]; if (module->create == NULL) { return Error( "Error creating module instance for '" + moduleName + "': " "create() method not found"); } std::string expectedKind = kind<T>(); if (expectedKind != module->kind) { return Error( "Error creating module instance for '" + moduleName + "': " "module is of kind '" + module->kind + "', but the requested " "kind is '" + expectedKind + "'"); } T* instance = module->create(params); if (instance == NULL) { return Error("Error creating Module instance for '" + moduleName + "'"); } return instance; } } {code} Then the original {{create()}} method can be simplified to: {code} template <typename T> static Try<T*> ModuleManager<T>::create(const std::string& moduleName) { return create(moduleName, moduleParameters[moduleName]); } {code} With this we can keep a simple version of {{ParameterFoo}}: {code} class ParameterFoo { public: Try<Foo*> create(int i) { return new ParameterFoo(i); } Try<Foo*> create(const Parameters& params) { Option<int> param = None; std::size_t error; for (const auto& param : params.parameter()) { if (param.key() == "i") { param = std::stoi(param.value(), &error); if (error == 0) { return Error("Could not parse parameters"); } } } if (param.isNone()) { return Error("Wrong type given in the parameters"); } return create(param.get()); } ParameterFoo(int i) : i_(i) {} virtual Future<int> hello() { return i; } private: int i_; }; {code} Some changes in {{tests::Module}} will be needed, adding an overload for create: {code} template<typename T> static Try<T*> tests::Module<T>::create(const Parameters& params) { Try<std::string> moduleName = getModuleName(N); if (moduleName.isError()) { return Error(moduleName.error()); } return mesos::modules::ModuleManager::create<T>(moduleName.get(), params); } {code} The test can thus be written as: {code} typedef ::testing::Types<ParameterFoo, tests::Module<Foo, TestParameterFoo>> FooTestTypes; TYPED_TEST_CASE(FooTest, FooTestTypes); TYPED_TEST(FooTest, ATest) { int fooValue = 1; // This part can go in the fixture set up Parameters params; Parameter* param = params.add_parameter(); param->set_key("i"); param->set_value(std::to_string(fooValue)); Try<Foo*> foo = TypeParam::create(params); ASSERT_SOME(foo); AWAIT_CHECK_EQUAL(foo.get()->hello(), fooValue); } {code} > Allow developers the option to pass parameters to modules programatically > ------------------------------------------------------------------------- > > Key: MESOS-3248 > URL: https://issues.apache.org/jira/browse/MESOS-3248 > Project: Mesos > Issue Type: Bug > Components: modules > Reporter: 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<>}}, however it is a common situation to require > initialization parameters for objects, however this constraint forces default > implementations of modularized components to have default constructors. > Module only implementations are allowed to have constructor parameters, since > the actual signature of their factory method is: > {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. However, this call is wrapped > by > {{ModuleManager}} which only allows module parameters to be passed from the > command line and do 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 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)