About ConfigurationPerformer in James

[Matthieu Baechler <ma...@apache.org>]

Hi there,

I read this PR this afternoon 
https://github.com/linagora/james-project/pull/2398 and I found a
recurring problem here: 
https://github.com/linagora/james-project/pull/2398/files#diff-34220ca9b8dffd3a398bb2b3ce17c81eR108

As it's a problem that occurs frequently and pass reviews easily, I
think that it deserves an explanation about what is the expected design
because doing such errors probably proves that the explanations are
lacking.

Let's first explore the problem.

Problem 1.

Years ago, James was using Spring as the only way to do dependency
injection.

And it provides a nice feature call @PostConstruct. This annotation
allows to start your application it two phases: the first is injection,
the second is components initialization.

Guice doesn't offer that feature out of the box. So we tried to
implement that feature we some smarts use of Guice features (and as
everybody knows, trying to be smart is very often a very good way to
make things too hard understand).

To implement this @PostConstruct substitute, we created some classes
extending ConfigurationPerformer that would just call whatever methods
are useful to initialize components.

These ConfigurationPerformer classes are then added to a Guice
multibinder and as a last step, in server start method, we call these
ConfigurationPerformers.

It worked for some time and then it failed.

Problem 2.

Initialization is supposed to respect dependencies ordering. So calling
ConfigurationPerformers can fail because an object of type A is calling
an object of type B before B is initiliazed.

The solution is to listen to type injection to build a List (a List
keep insertion ordering) of the types that are injected in the order
they are created. As the injection framework creates objects with
respect to the dependency hierarchy, we then end up with the sequence
we need to respect for the initialization phase.

But this only give us the List of objects to initialize and not the
objects that perform the init. So we iterate the List and for each
element we try to find the matching ConfigurationPerformers. For this
to work, every ConfigurationPerformer has to declare what are the
classes it initializes with the famous forClasses method. For each
ConfigurationPerformer, we finally call the init method.

For the specific situation linked at the top of this email, the
ConfigurationPerformer declares MailboxIndexCreator as the type it
initializes. But MailboxIndexCreator is not injected anywhere, so it
will be created without any specific ordering.

The truth is: we want indices creation to happen before we start
objects that depend on an ElasticSearch connection, otherwise, the
indices may not exist in time for this object to work. So in this case,
the solution is quite simple: we probably have to declare
RestHighLevelClient or something related in the forClass of the
ConfigurationPerformer.

One last point that we have to look at is the interaction with
StartupCheck: do we want to try to create indices before checking
ElasticSearch version? (I guess not).

Thanks for reading and don't hesitate to ask if you need some more
explanations.

--
Matthieu Baechler


---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org

Re: About ConfigurationPerformer in James

[Antoine DUPRAT <ad...@linagora.com>]

What about doing such initialization features (DB creation, indices 
creation...) before Guice injection.
This is not easy, and may need to handle a double injection.

The advantages are:
- the initialization can be configured (play it or not)
- we can start James with minimal services for administrators only (in 
case of some failure)
- we will have a better feedback of the time spent in initialization 
between real injection


Antoine DUPRAT DevOps CI/CD Tél : +33 6 45 63 27 17 Courriel : 
aduprat@linagora.com www.linagora.com
Le 03/06/2019 à 16:45, Matthieu Baechler a écrit :
> Hi there,
>
> I read this PR this afternoon
> https://github.com/linagora/james-project/pull/2398 and I found a
> recurring problem here:
> https://github.com/linagora/james-project/pull/2398/files#diff-34220ca9b8dffd3a398bb2b3ce17c81eR108
>
> As it's a problem that occurs frequently and pass reviews easily, I
> think that it deserves an explanation about what is the expected design
> because doing such errors probably proves that the explanations are
> lacking.
>
> Let's first explore the problem.
>
> Problem 1.
>
> Years ago, James was using Spring as the only way to do dependency
> injection.
>
> And it provides a nice feature call @PostConstruct. This annotation
> allows to start your application it two phases: the first is injection,
> the second is components initialization.
>
> Guice doesn't offer that feature out of the box. So we tried to
> implement that feature we some smarts use of Guice features (and as
> everybody knows, trying to be smart is very often a very good way to
> make things too hard understand).
>
> To implement this @PostConstruct substitute, we created some classes
> extending ConfigurationPerformer that would just call whatever methods
> are useful to initialize components.
>
> These ConfigurationPerformer classes are then added to a Guice
> multibinder and as a last step, in server start method, we call these
> ConfigurationPerformers.
>
> It worked for some time and then it failed.
>
> Problem 2.
>
> Initialization is supposed to respect dependencies ordering. So calling
> ConfigurationPerformers can fail because an object of type A is calling
> an object of type B before B is initiliazed.
>
> The solution is to listen to type injection to build a List (a List
> keep insertion ordering) of the types that are injected in the order
> they are created. As the injection framework creates objects with
> respect to the dependency hierarchy, we then end up with the sequence
> we need to respect for the initialization phase.
>
> But this only give us the List of objects to initialize and not the
> objects that perform the init. So we iterate the List and for each
> element we try to find the matching ConfigurationPerformers. For this
> to work, every ConfigurationPerformer has to declare what are the
> classes it initializes with the famous forClasses method. For each
> ConfigurationPerformer, we finally call the init method.
>
> For the specific situation linked at the top of this email, the
> ConfigurationPerformer declares MailboxIndexCreator as the type it
> initializes. But MailboxIndexCreator is not injected anywhere, so it
> will be created without any specific ordering.
>
> The truth is: we want indices creation to happen before we start
> objects that depend on an ElasticSearch connection, otherwise, the
> indices may not exist in time for this object to work. So in this case,
> the solution is quite simple: we probably have to declare
> RestHighLevelClient or something related in the forClass of the
> ConfigurationPerformer.
>
> One last point that we have to look at is the interaction with
> StartupCheck: do we want to try to create indices before checking
> ElasticSearch version? (I guess not).
>
> Thanks for reading and don't hesitate to ask if you need some more
> explanations.
>
> --
> Matthieu Baechler
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
> For additional commands, e-mail: server-dev-help@james.apache.org
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org