Does ‘StartupControlPanel#loadStartupLoaders’ really require introspection?

[Mathieu Lirzin]

Hello,

I have a question regarding the
‘org.apache.ofbiz.base.start.StartupControlPanel#loadStartupLoaders()’
current implementation:

--8<---------------cut here---------------start------------->8---
    private static void loadStartupLoaders(Config config,
            List<StartupLoader> loaders,
            List<StartupCommand> ofbizCommands,
            AtomicReference<ServerState> serverState) throws StartupException {

        String startupLoaderName = "org.apache.ofbiz.base.container.ContainerLoader";
        ClassLoader classloader = Thread.currentThread().getContextClassLoader();

        synchronized (loaders) {
            if (serverState.get() == ServerState.STOPPING) {
                return;
            }
            try {
                Class<?> loaderClass = classloader.loadClass(startupLoaderName);
                StartupLoader loader = (StartupLoader) loaderClass.newInstance();
                loaders.add(loader); // add before loading, so unload can occur if error during loading
                loader.load(config, ofbizCommands);
            } catch (ReflectiveOperationException e) {
                throw new StartupException(e);
            }
        }
        serverState.compareAndSet(ServerState.STARTING, ServerState.RUNNING);
    }
--8<---------------cut here---------------end--------------->8---

I don't understand the goal of using reflection for instantiating the
‘ContainerLoader’ class. Can't we just have something like the following
instead?

--8<---------------cut here---------------start------------->8---
    private static void loadStartupLoaders(Config config,
            List<StartupLoader> loaders,
            List<StartupCommand> ofbizCommands,
            AtomicReference<ServerState> serverState) throws StartupException {

        ClassLoader classloader = Thread.currentThread().getContextClassLoader();

        synchronized (loaders) {
            if (serverState.get() == ServerState.STOPPING) {
                return;
            }
            StartupLoader loader = new ContainerLoader();
            loaders.add(loader); // add before loading, so unload can occur if error during loading
            loader.load(config, ofbizCommands);
        }
        serverState.compareAndSet(ServerState.STARTING, ServerState.RUNNING);
    }
--8<---------------cut here---------------end--------------->8---

If this is required, I will be happy if someone could add an inline
comment giving a rationale for the current implementation.  If that's
not the case, I can open a JIRA with a proper patch if needed.

Thanks.

--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37

[taher]

Hi Mathieu,

This is code which I refactored from a very messy startup code, and
yes you are right, we don't need java reflection, and we do not even
need a startup loader in the first place. The code could be much
simpler.

We used to have 2 startup loaders, the existing one, and also a loader
for the GUI POS system which we deleted during refactoring. The idea
back in the day was that the startup code could be instantiated by
multiple loaders to customize the behavior of the system.

The refactoring work is not yet done, and we got side tracked by many
issues, but I would say maybe 50% or more of the existing java code
base could be trimmed down and simplified.

My recommendation is to completely delete the startup loaders API, but
there are a few technical challenges to it because there is some state
linked to it here and there, so it's a matter of catching everything,
re-routing ,and getting a simpler startup logic.
On Mon, Oct 22, 2018 at 12:37 AM Mathieu Lirzin
<[hidden email]> wrote:

>
> Hello,
>
> I have a question regarding the
> ‘org.apache.ofbiz.base.start.StartupControlPanel#loadStartupLoaders()’
> current implementation:
>
> --8<---------------cut here---------------start------------->8---
>     private static void loadStartupLoaders(Config config,
>             List<StartupLoader> loaders,
>             List<StartupCommand> ofbizCommands,
>             AtomicReference<ServerState> serverState) throws StartupException {
>
>         String startupLoaderName = "org.apache.ofbiz.base.container.ContainerLoader";
>         ClassLoader classloader = Thread.currentThread().getContextClassLoader();
>
>         synchronized (loaders) {
>             if (serverState.get() == ServerState.STOPPING) {
>                 return;
>             }
>             try {
>                 Class<?> loaderClass = classloader.loadClass(startupLoaderName);
>                 StartupLoader loader = (StartupLoader) loaderClass.newInstance();
>                 loaders.add(loader); // add before loading, so unload can occur if error during loading
>                 loader.load(config, ofbizCommands);
>             } catch (ReflectiveOperationException e) {
>                 throw new StartupException(e);
>             }
>         }
>         serverState.compareAndSet(ServerState.STARTING, ServerState.RUNNING);
>     }
> --8<---------------cut here---------------end--------------->8---
>
> I don't understand the goal of using reflection for instantiating the
> ‘ContainerLoader’ class. Can't we just have something like the following
> instead?
>
> --8<---------------cut here---------------start------------->8---
>     private static void loadStartupLoaders(Config config,
>             List<StartupLoader> loaders,
>             List<StartupCommand> ofbizCommands,
>             AtomicReference<ServerState> serverState) throws StartupException {
>
>         ClassLoader classloader = Thread.currentThread().getContextClassLoader();
>
>         synchronized (loaders) {
>             if (serverState.get() == ServerState.STOPPING) {
>                 return;
>             }
>             StartupLoader loader = new ContainerLoader();
>             loaders.add(loader); // add before loading, so unload can occur if error during loading
>             loader.load(config, ofbizCommands);
>         }
>         serverState.compareAndSet(ServerState.STARTING, ServerState.RUNNING);
>     }
> --8<---------------cut here---------------end--------------->8---
>
> If this is required, I will be happy if someone could add an inline
> comment giving a rationale for the current implementation.  If that's
> not the case, I can open a JIRA with a proper patch if needed.
>
> Thanks.
>
> --
> Mathieu Lirzin
> GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37

[Mathieu Lirzin]

Hello Taher,

Taher Alkhateeb <[hidden email]> writes:

> This is code which I refactored from a very messy startup code, and
> yes you are right, we don't need java reflection, and we do not even
> need a startup loader in the first place. The code could be much
> simpler.
>
> We used to have 2 startup loaders, the existing one, and also a loader
> for the GUI POS system which we deleted during refactoring. The idea
> back in the day was that the startup code could be instantiated by
> multiple loaders to customize the behavior of the system.
>
> The refactoring work is not yet done, and we got side tracked by many
> issues, but I would say maybe 50% or more of the existing java code
> base could be trimmed down and simplified.
>
> My recommendation is to completely delete the startup loaders API, but
> there are a few technical challenges to it because there is some state
> linked to it here and there, so it's a matter of catching everything,
> re-routing ,and getting a simpler startup logic.

Excellent, thanks for giving me more background on the current
implementation.  I have been looking at OFBIZ-8337 which seems related
and provides even more details.  :-)

I will be looking into deleting the startup loaders API.

Thanks.

--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37

[Mathieu Lirzin]

Hello,

Mathieu Lirzin <[hidden email]> writes:

> I will be looking into deleting the startup loaders API.

As a citizen of the *Start-Up Nation* [1], I am proud to announce that
the startup loader abstraction has finally been concretized! :-)

I have opened OFBIZ-10638 [2] for review.

Thanks.

[1] https://www.nytimes.com/2018/05/23/business/emmanuel-macron-france-technology.html
[2] https://issues.apache.org/jira/browse/OFBIZ-10638

--
Mathieu Lirzin
GPG: F2A3 8D7E EB2B 6640 5761  070D 0ADE E100 9460 4D37