Agent: Add possibility to Inject Custom Logger and Dispatcher, reasonable defaults otherwise

[lsulak]

Background

As the ticket's title says - currently the Agent's constructor is private and it should stay like that. But there is a companion object associated with it - it can have a function called apply that would instantiate the agent. We can use this for user to have a possibility to inject logging and dispatcher as he wants. (Not completely sure about the dispatcher though - TO discuss).

If it's not specified, a reasonable defaults should be used.

[salamonpavel]

I think the companion pattern doesn't fit this use case. Firstly once your AtumAgent companion object extends the AtumAgent class you need to pass the parameters to the constuctor of AtumAgent. Moreover module extending its companion class cannot use default constructor arguments (not a problem if you define the defaults in the apply method on the companion).

object AtumAgent extends AtumAgent(/** here goes parameters **/)

I would instead propose to expose AtumAgentBuilder to the client code via which AtumAgent can be instantiated. This will allow us to define resonable defaults for AtumAgent's fields and the API will be in my opinion more friendly.

class AtumAgent private[agent] (private val maybeDispatcher: Option[Dispatcher]) {

  val config: Config = ConfigFactory.load()

  private val dispatcher: Dispatcher = maybeDispatcher.getOrElse {
    config.getString("atum.dispatcher.type") match {
      case "http" => new HttpDispatcher(config.getConfig("atum.dispatcher.http"))
      case "console" => new ConsoleDispatcher
      case dt => throw new UnsupportedOperationException(s"Unsupported dispatcher type: '$dt''")
    }
  }
/** rest of the body **/
}

case class AtumAgentBuilder private (dispatcher: Option[Dispatcher] = None) {
  private [agent] def withDispatcher(dispatcher: Dispatcher): AtumAgentBuilder = copy(dispatcher = Some(dispatcher))
  def build(): AtumAgent = new AtumAgent(dispatcher)
}

Also I wonder if we really want to allow the client code supply the dispatcher at this point or use private [agent] access modifier.

[cerveada]

I agree this could be changed, but I would do it like this:

class AtumAgent private[agent] (dispatcher: Dispatcher) {
  /** rest of the body **/
}

object AtumAgent {
  def apply() = {
      val config: Config = ConfigFactory.load()
      val dispatcher = config.getString("atum.dispatcher.type") match {
          case "http" => new HttpDispatcher(config.getConfig("atum.dispatcher.http"))
          case "console" => new ConsoleDispatcher
          case dt => throw new UnsupportedOperationException(s"Unsupported dispatcher type: '$dt''")
      new AtumAgent(dispatcher)
  }
}

• defaults are in reference.conf
• agent clearly defines what it needs (the dispatcher)
• mock dispatcher can be injected for testing without need for another agent implementation
• no need for null or Option
• as I understand it apply methods in companion objects are basically scala way of implementing builders.

[salamonpavel]

I agree this could be changed, but I would do it like this:

class AtumAgent private[agent] (dispatcher: Dispatcher) {
  /** rest of the body **/
}

object AtumAgent {
  def apply() = {
      val config: Config = ConfigFactory.load()
      val dispatcher = config.getString("atum.dispatcher.type") match {
          case "http" => new HttpDispatcher(config.getConfig("atum.dispatcher.http"))
          case "console" => new ConsoleDispatcher
          case dt => throw new UnsupportedOperationException(s"Unsupported dispatcher type: '$dt''")
      new AtumAgent(dispatcher)
  }
}

* defaults are in `reference.conf`

* agent clearly defines what it needs (the dispatcher)

* mock dispatcher can be injected for testing without need for another agent implementation

* no need for null or Option

* as I understand it apply methods in companion objects are basically scala way of implementing builders.

I like the point about not having to use Option.
When we have a need to allow for custom logger (and potentially other dependencies) injection you would create yet another apply method(s)?

[cerveada]

When we have a need to allow for custom logger (and potentially other dependencies) injection you would create yet another apply method(s)?

Why not add another config key the same way it's done for dispatcher? We can even accept class name in the config and then instantiate that class when needed.

But we can also add another apply method of course.

[benedeki]

The main question is, for what purpose we want to inject the custom dispatcher and logger.

• if only for testing, I think it's enough to add them as input parameters of the class AtumAgent
• if we want the users to do that too, @cerveada suggestion is pretty good imho.

Just a note, I would like to keep object AtumAgent extends AtumAgent so it's not needed for the user to spawn the instance.