[jira] [Comment Edited] (MESOS-3248) Allow developers the option to pass parameters to modules programatically

["Alexander Rojas (JIRA)" <ji...@apache.org>]

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