[jira] [Commented] (OFBIZ-10638) The ‘StartupLoader’ interface should be removed
[
https://issues.apache.org/jira/browse/OFBIZ-10638?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16759513#comment-16759513
]
Mathieu Lirzin commented on OFBIZ-10638:
Thanks alot Taher!
> The ‘StartupLoader’ interface should be removed
> ---
>
> Key: OFBIZ-10638
> URL: https://issues.apache.org/jira/browse/OFBIZ-10638
> Project: OFBiz
> Issue Type: Improvement
> Components: framework
>Affects Versions: Trunk
>Reporter: Mathieu Lirzin
>Assignee: Taher Alkhateeb
>Priority: Minor
> Fix For: Upcoming Branch
>
> Attachments: OFBIZ-10638_Remove-StartupLoader-interface.patch
>
>
> OFBiz used to provide alternate Startup loaders. Nowadays only the container
> loaders is used. As suggested by Taher [on the dev mailing
> list|https://lists.apache.org/thread.html/f99d6f661eb8197df8eac6d8ba7db3fa9b7fe2569a4a24ef2fef5cae@%3Cdev.ofbiz.apache.org%3E],
> the {{StartupLoader}} interface should be removed and startup code using it
> should be adapted.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (OFBIZ-10638) The ‘StartupLoader’ interface should be removed
[
https://issues.apache.org/jira/browse/OFBIZ-10638?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16755058#comment-16755058
]
Mathieu Lirzin commented on OFBIZ-10638:
I am trying to preserve previous behavior. :-)
An atomic reference on the server state ensures atomicity of mutation of that
reference only, but can't ensure the coordination of multiple thread safe
mutations. To achieve that you need an extra synchronization layer, which in
some case make the individual atomicity mechanism redundant.
In the above snippet both the loader and the server state are thread safe
references on their own, but without a synchronized block (or another
synchronization mechanism) you can't ensure for example that loading will be
done only when the server state is either STARTING or RUNNING which was a
concurrency warranty of the initial implementation.
I noticed potential concurrency issues in the initial implementation (the major
one being that the invariants that should be preserved are not clearly
documented) and to be honest I am currently working on that in parallel.
However my strategy for this ticket is to consider the removal of the
{{StartupLoader}} interface in isolation. Meaning I want to postpone
fixing/improving prior concurrency issue to another ticket. I only want to make
sure that I don't introduce more potential concurrency bugs by reducing the
change I make to the minimum. I think it will work better for both of us if we
stick to this separation of concerns. Are you OK with that?
> The ‘StartupLoader’ interface should be removed
> ---
>
> Key: OFBIZ-10638
> URL: https://issues.apache.org/jira/browse/OFBIZ-10638
> Project: OFBiz
> Issue Type: Improvement
> Components: framework
>Affects Versions: Trunk
>Reporter: Mathieu Lirzin
>Assignee: Taher Alkhateeb
>Priority: Minor
> Fix For: Upcoming Branch
>
> Attachments: OFBIZ-10638_Remove-StartupLoader-interface.patch
>
>
> OFBiz used to provide alternate Startup loaders. Nowadays only the container
> loaders is used. As suggested by Taher [on the dev mailing
> list|https://lists.apache.org/thread.html/f99d6f661eb8197df8eac6d8ba7db3fa9b7fe2569a4a24ef2fef5cae@%3Cdev.ofbiz.apache.org%3E],
> the {{StartupLoader}} interface should be removed and startup code using it
> should be adapted.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (OFBIZ-10638) The ‘StartupLoader’ interface should be removed
[
https://issues.apache.org/jira/browse/OFBIZ-10638?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16754888#comment-16754888
]
Taher Alkhateeb commented on OFBIZ-10638:
-
Great book to read :)
Either way, I'm not entirely sure what you're trying to preserve? What are the
threads competing for? The OFBiz server is a singleton instance. Where is the
conflict? Furthermore, the the server state is already an atomic reference. Why
make a lock to update an atomic operation anyway?
> The ‘StartupLoader’ interface should be removed
> ---
>
> Key: OFBIZ-10638
> URL: https://issues.apache.org/jira/browse/OFBIZ-10638
> Project: OFBiz
> Issue Type: Improvement
> Components: framework
>Affects Versions: Trunk
>Reporter: Mathieu Lirzin
>Assignee: Taher Alkhateeb
>Priority: Minor
> Fix For: Upcoming Branch
>
> Attachments: OFBIZ-10638_Remove-StartupLoader-interface.patch
>
>
> OFBiz used to provide alternate Startup loaders. Nowadays only the container
> loaders is used. As suggested by Taher [on the dev mailing
> list|https://lists.apache.org/thread.html/f99d6f661eb8197df8eac6d8ba7db3fa9b7fe2569a4a24ef2fef5cae@%3Cdev.ofbiz.apache.org%3E],
> the {{StartupLoader}} interface should be removed and startup code using it
> should be adapted.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (OFBIZ-10638) The ‘StartupLoader’ interface should be removed
[
https://issues.apache.org/jira/browse/OFBIZ-10638?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16754876#comment-16754876
]
Mathieu Lirzin commented on OFBIZ-10638:
Hello Taher,
After studying “Java concurrency in practice” by Brian Goetz, I have updated
[^OFBIZ-10638_Remove-StartupLoader-interface.patch] with the following change.
The initial code for {{loadStartupLoaders}} with multiple loaders was the
following:
{code:java}
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);
}
}
{code}
my first patch was relaxing the invariants by removing the synchronization
between the check of the server state and the loading of the component
container:
{code:java}
if (serverState.get() == ServerState.STOPPING) {
return;
}
loader.load(config, ofbizCommands);
{code}
the updated patch reintroduce that invariant to preserve the initial
concurrency semantics
{code:java}
synchronized (StartupControlPanel.class) {
if (serverState.get() == ServerState.STOPPING) {
return;
}
loader.load(config, ofbizCommands);
}
{code}
To avoid delaying the close of that ticket, I have removed the extra
refactoring patches which I keep for a later ticket.
> The ‘StartupLoader’ interface should be removed
> ---
>
> Key: OFBIZ-10638
> URL: https://issues.apache.org/jira/browse/OFBIZ-10638
> Project: OFBiz
> Issue Type: Improvement
>Affects Versions: Trunk
>Reporter: Mathieu Lirzin
>Assignee: Taher Alkhateeb
>Priority: Minor
> Fix For: Upcoming Branch
>
> Attachments: OFBIZ-10638_Remove-StartupLoader-interface.patch
>
>
> OFBiz used to provide alternate Startup loaders. Nowadays only the container
> loaders is used. As suggested by Taher [on the dev mailing
> list|https://lists.apache.org/thread.html/f99d6f661eb8197df8eac6d8ba7db3fa9b7fe2569a4a24ef2fef5cae@%3Cdev.ofbiz.apache.org%3E],
> the {{StartupLoader}} interface should be removed and startup code using it
> should be adapted.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (OFBIZ-10638) The ‘StartupLoader’ interface should be removed
[
https://issues.apache.org/jira/browse/OFBIZ-10638?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16754874#comment-16754874
]
Taher Alkhateeb commented on OFBIZ-10638:
-
Hi Mathieu,
It's been a while since I reviewed this ticket, but why did you re-apply the
object lock?
> The ‘StartupLoader’ interface should be removed
> ---
>
> Key: OFBIZ-10638
> URL: https://issues.apache.org/jira/browse/OFBIZ-10638
> Project: OFBiz
> Issue Type: Improvement
>Affects Versions: Trunk
>Reporter: Mathieu Lirzin
>Assignee: Taher Alkhateeb
>Priority: Minor
> Fix For: Upcoming Branch
>
> Attachments: OFBIZ-10638_Remove-StartupLoader-interface.patch
>
>
> OFBiz used to provide alternate Startup loaders. Nowadays only the container
> loaders is used. As suggested by Taher [on the dev mailing
> list|https://lists.apache.org/thread.html/f99d6f661eb8197df8eac6d8ba7db3fa9b7fe2569a4a24ef2fef5cae@%3Cdev.ofbiz.apache.org%3E],
> the {{StartupLoader}} interface should be removed and startup code using it
> should be adapted.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (OFBIZ-10638) The ‘StartupLoader’ interface should be removed
[
https://issues.apache.org/jira/browse/OFBIZ-10638?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16707499#comment-16707499
]
Taher Alkhateeb commented on OFBIZ-10638:
-
Hi Mathieu,
Sorry for the delay on this. I will work on it soon. FYI if you want to push
things forward you can nudge me here and I'll get back to it.
> The ‘StartupLoader’ interface should be removed
> ---
>
> Key: OFBIZ-10638
> URL: https://issues.apache.org/jira/browse/OFBIZ-10638
> Project: OFBiz
> Issue Type: Improvement
>Affects Versions: Trunk
>Reporter: Mathieu Lirzin
>Assignee: Taher Alkhateeb
>Priority: Minor
> Fix For: Upcoming Branch
>
> Attachments: OFBIZ-10638_0001-Remove-StartupLoader-interface.patch,
> OFBIZ-10638_0002-Remove-unnecessary-arguments-in-AdminServer.patch,
> OFBIZ-10638_0003-Refactor-StartupControlPanel.patch,
> OFBIZ-10638_0004-Refactor-ContainerLoader.patch
>
>
> OFBiz used to provide alternate Startup loaders. Nowadays only the container
> loaders is used. As suggested by Taher [on the dev mailing
> list|https://lists.apache.org/thread.html/f99d6f661eb8197df8eac6d8ba7db3fa9b7fe2569a4a24ef2fef5cae@%3Cdev.ofbiz.apache.org%3E],
> the {{StartupLoader}} interface should be removed and startup code using it
> should be adapted.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (OFBIZ-10638) The ‘StartupLoader’ interface should be removed
[
https://issues.apache.org/jira/browse/OFBIZ-10638?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16691489#comment-16691489
]
Mathieu Lirzin commented on OFBIZ-10638:
My understanding is that the synchronization mechanism I removed was only
present to ensure that the manipulation of the {{loaders}} list was done
atomically. It should not affect the loading/unloading of containers since
those operations are still done by synchronized methods in the
{{ContainerLoader}} class. The reason I removed the synchronized block is that
there is no list to manipulate anymore.
After installing all the birt plugin the birt container is loading, here is the
log:
{code}
2018-11-19 09:40:00,047 |main |ContainerLoader
|I| Starting container catalina-container
...
2018-11-19 09:40:07,835 |main |ContainerLoader
|I| Started container catalina-container
2018-11-19 09:40:07,835 |main |ContainerLoader
|I| Starting container birt-container
...
2018-11-19 09:40:09,229 |main |ContainerLoader
|I| Started container birt-container
{code}
> The ‘StartupLoader’ interface should be removed
> ---
>
> Key: OFBIZ-10638
> URL: https://issues.apache.org/jira/browse/OFBIZ-10638
> Project: OFBiz
> Issue Type: Improvement
>Affects Versions: Trunk
>Reporter: Mathieu Lirzin
>Assignee: Taher Alkhateeb
>Priority: Minor
> Fix For: Upcoming Branch
>
> Attachments: OFBIZ-10638_0001-Remove-StartupLoader-interface.patch,
> OFBIZ-10638_0002-Remove-unnecessary-arguments-in-AdminServer.patch,
> OFBIZ-10638_0003-Refactor-StartupControlPanel.patch,
> OFBIZ-10638_0004-Refactor-ContainerLoader.patch
>
>
> OFBiz used to provide alternate Startup loaders. Nowadays only the container
> loaders is used. As suggested by Taher [on the dev mailing
> list|https://lists.apache.org/thread.html/f99d6f661eb8197df8eac6d8ba7db3fa9b7fe2569a4a24ef2fef5cae@%3Cdev.ofbiz.apache.org%3E],
> the {{StartupLoader}} interface should be removed and startup code using it
> should be adapted.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (OFBIZ-10638) The ‘StartupLoader’ interface should be removed
[
https://issues.apache.org/jira/browse/OFBIZ-10638?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16690914#comment-16690914
]
Taher Alkhateeb commented on OFBIZ-10638:
-
Well, the syncrhonized block around startup loaders is now converted to a
single container loader. This means we removed the synchronization mechanism
for the startup loaders yet we continued to launch all the containers as usual.
I think your code looks fine, but just to be on the safe side, can you confirm
that all containers are launched (anything that implements container). Does
that for example include the birt container if you add that plugin? No
problems? You know, just some smoke tests to make sure we are okay
> The ‘StartupLoader’ interface should be removed
> ---
>
> Key: OFBIZ-10638
> URL: https://issues.apache.org/jira/browse/OFBIZ-10638
> Project: OFBiz
> Issue Type: Improvement
>Affects Versions: Trunk
>Reporter: Mathieu Lirzin
>Assignee: Taher Alkhateeb
>Priority: Minor
> Fix For: Upcoming Branch
>
> Attachments: OFBIZ-10638_0001-Remove-StartupLoader-interface.patch,
> OFBIZ-10638_0002-Remove-unnecessary-arguments-in-AdminServer.patch,
> OFBIZ-10638_0003-Refactor-StartupControlPanel.patch,
> OFBIZ-10638_0004-Refactor-ContainerLoader.patch
>
>
> OFBiz used to provide alternate Startup loaders. Nowadays only the container
> loaders is used. As suggested by Taher [on the dev mailing
> list|https://lists.apache.org/thread.html/f99d6f661eb8197df8eac6d8ba7db3fa9b7fe2569a4a24ef2fef5cae@%3Cdev.ofbiz.apache.org%3E],
> the {{StartupLoader}} interface should be removed and startup code using it
> should be adapted.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (OFBIZ-10638) The ‘StartupLoader’ interface should be removed
[
https://issues.apache.org/jira/browse/OFBIZ-10638?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16690913#comment-16690913
]
Mathieu Lirzin commented on OFBIZ-10638:
Hello Taher,
Indeed it is critical code, so you are right by being cautious.
Both manual tests, unit tests, integration tests, and data loading work fine on
my machine. I am not a Java concurrency expert, so I can only be *relatively*
confident with patch 1.
What do you mean by investigating the variable state for the container loader?
> The ‘StartupLoader’ interface should be removed
> ---
>
> Key: OFBIZ-10638
> URL: https://issues.apache.org/jira/browse/OFBIZ-10638
> Project: OFBiz
> Issue Type: Improvement
>Affects Versions: Trunk
>Reporter: Mathieu Lirzin
>Assignee: Taher Alkhateeb
>Priority: Minor
> Fix For: Upcoming Branch
>
> Attachments: OFBIZ-10638_0001-Remove-StartupLoader-interface.patch,
> OFBIZ-10638_0002-Remove-unnecessary-arguments-in-AdminServer.patch,
> OFBIZ-10638_0003-Refactor-StartupControlPanel.patch,
> OFBIZ-10638_0004-Refactor-ContainerLoader.patch
>
>
> OFBiz used to provide alternate Startup loaders. Nowadays only the container
> loaders is used. As suggested by Taher [on the dev mailing
> list|https://lists.apache.org/thread.html/f99d6f661eb8197df8eac6d8ba7db3fa9b7fe2569a4a24ef2fef5cae@%3Cdev.ofbiz.apache.org%3E],
> the {{StartupLoader}} interface should be removed and startup code using it
> should be adapted.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (OFBIZ-10638) The ‘StartupLoader’ interface should be removed
[
https://issues.apache.org/jira/browse/OFBIZ-10638?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16690456#comment-16690456
]
Taher Alkhateeb commented on OFBIZ-10638:
-
Hello Mathieu, Thank you for your work so far, and my apology for not picking
this up earlier.
I was just reviewing patch 1, and I remember doing something similar in the
past but I think I got a problem around thread synchronization and access to
variables. Are you confident of your code in patch1? Did you apply initial
smoke tests? Are all the containers running okay? Did you investigate the
variable state for the container loader? Did you test multiple scenarios like
running data loading, running integration tests and all of that? Everything
okay?
Sorry for the too many questions, but this is critical code and so we should
tread carefully and slowly.
> The ‘StartupLoader’ interface should be removed
> ---
>
> Key: OFBIZ-10638
> URL: https://issues.apache.org/jira/browse/OFBIZ-10638
> Project: OFBiz
> Issue Type: Improvement
>Affects Versions: Trunk
>Reporter: Mathieu Lirzin
>Assignee: Taher Alkhateeb
>Priority: Minor
> Fix For: Upcoming Branch
>
> Attachments: OFBIZ-10638_0001-Remove-StartupLoader-interface.patch,
> OFBIZ-10638_0002-Remove-unnecessary-arguments-in-AdminServer.patch,
> OFBIZ-10638_0003-Refactor-StartupControlPanel.patch,
> OFBIZ-10638_0004-Refactor-ContainerLoader.patch
>
>
> OFBiz used to provide alternate Startup loaders. Nowadays only the container
> loaders is used. As suggested by Taher [on the dev mailing
> list|https://lists.apache.org/thread.html/f99d6f661eb8197df8eac6d8ba7db3fa9b7fe2569a4a24ef2fef5cae@%3Cdev.ofbiz.apache.org%3E],
> the {{StartupLoader}} interface should be removed and startup code using it
> should be adapted.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (OFBIZ-10638) The ‘StartupLoader’ interface should be removed
[
https://issues.apache.org/jira/browse/OFBIZ-10638?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16673668#comment-16673668
]
Mathieu Lirzin commented on OFBIZ-10638:
I have provided [^OFBIZ-10638_0001-Remove-StartupLoader-interface.patch] which
is doing the actual job of removing the {{StartupLoader}} interface.
Additionnally
[^OFBIZ-10638_0002-Remove-unnecessary-arguments-in-AdminServer.patch] ,
[^OFBIZ-10638_0003-Refactor-StartupControlPanel.patch] , and
[^OFBIZ-10638_0004-Refactor-ContainerLoader.patch] provides some extra code
cleanups I have made while hacking on the interface removal.
Thoses patches are meant to be apply in order.
> The ‘StartupLoader’ interface should be removed
> ---
>
> Key: OFBIZ-10638
> URL: https://issues.apache.org/jira/browse/OFBIZ-10638
> Project: OFBiz
> Issue Type: Improvement
>Affects Versions: Trunk
>Reporter: Mathieu Lirzin
>Priority: Minor
> Fix For: Upcoming Branch
>
> Attachments: OFBIZ-10638_0001-Remove-StartupLoader-interface.patch,
> OFBIZ-10638_0002-Remove-unnecessary-arguments-in-AdminServer.patch,
> OFBIZ-10638_0003-Refactor-StartupControlPanel.patch,
> OFBIZ-10638_0004-Refactor-ContainerLoader.patch
>
>
> OFBiz used to provide alternate Startup loaders. Nowadays only the container
> loaders is used. As suggested by Taher [on the dev mailing
> list|https://lists.apache.org/thread.html/f99d6f661eb8197df8eac6d8ba7db3fa9b7fe2569a4a24ef2fef5cae@%3Cdev.ofbiz.apache.org%3E],
> the {{StartupLoader}} interface should be removed and startup code using it
> should be adapted.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
