Hi Clebert,
I created the pull request.
https://github.com/apache/activemq-artemis/pull/243
While there are many changed files in the pull request I think the
impact on the API should be pretty small.
The only real API change is the move of the XMLConfigurationUtil class.
All other changes are inside impl packages.
The problem with the API though is that the Artemis API is not self
contained. So it is difficult to judge what really breaks the user code.
I did some more comments inline. In any case I think the parts I touched
there should not be used by end users. So the wildfly integration
is probably the main thing possibly impacted and that can be tested I hope.
Are you involved in Wildfly or know some people? If yes it would be
great if you could check the impact.
I added some more comments inline.
Christian
On 16.11.2015 19:36, Clebert Suconic wrote:
a Pull request will be a great tool here!!! we could discuss on top of
actual changes.
On Mon, Nov 16, 2015 at 12:28 PM, Christian Schneider
<[email protected]> wrote:
In the scope of the OSGi enablement of artemis I am currently looking into
the split package org.apache.activemq.artemis.utils.
The immediate problem we have is that this package is also present in the
artemis-commons project. So one simple solution would be to just move it to
a package
like org.apache.activemq.artemis.server.config.utils.
The real problem is a bit deeper though.
XMLConfigurationUtil refers to
org.apache.activemq.artemis.core.config.impl.Validators
So a non impl package refers to an impl package which is already not ideal.
We could move it to common/util/xml, and create an interface for the
Validator.. so the impl could leave elsewhere and passed as a
paremeter:
Something along this line?
https://github.com/clebertsuconic/activemq-artemis/commit/09d95f6e29f3921a94bfe5a43942d51c321240cf
^^ that's just a suggestion.. and part of the discussion here. It
doesn't even compile as of right now.
(from a branch I called suggestion on my fork for artemis)
Extracting the Validator interface was also my first try. Unfortunately
the Actual Validator impls are also used in some places.
That is why I tried to make them also independent of the impl.
I agree though that just extracting the Validator interface would be a
smaller change. So if the bigger one can not be accepted then that would do.
//
XMLConfiguationUtil is used in:
- org.apache.activemq.artemis.core.config.impl.FileConfigurationParser
- org.apache.activemq.artemis.jms.server.config.impl.FileJMSConfiguration
So at least the impact of changes should be relatively small.
Additionally the Validators class refers to some other impl packages which
is also not so good.
import
Moving to util and creating the interface would fix that?
It would not fix the actual Validators but at least the direct
dependencies of XMLConfigurationUtil would be clean then.
org.apache.activemq.artemis.core.server.cluster.impl.MessageLoadBalancingType;
import
org.apache.activemq.artemis.core.settings.impl.AddressFullMessagePolicy;
import org.apache.activemq.artemis.core.settings.impl.SlowConsumerPolicy;
All these classes are enums. Strangely these are in impl packages. I would
have expected them to be in the repective API.
So I think it would be a good step to move these enums one level up to the
API packages.
Then Validators would only refer to other API packages which is already
quite a bit better.
Moving this would break the API, and the integration with user's and
applications. One obvious user will be the Wildfly.. We could of
course fix the integration here but that wouldn't fix anyone else
using the same API.
We could always move, extend it and deprecate the old package.
The move of the classes above should not break the API as they are in
the impl packages now.
Of course that only works if the impl are not also considered API.
As currently there are so many references from API to impl it is
difficult to say if people really just use the API.
As Validators and XMLConfigurationUtil are very closely related and deal
with XML config I propose to put them in the same package
org.apache.activemq.artemis.core.server.config.xml.
What do you think?
If you consider to do this change I can send a pull request.
Christian
Btw. I also found some other small odd thing:
org.apache.activemq.artemis.core.deployers.impl.FileConfigurationParser
seems to be misplaced.
It seems to rather belong into org.apache.activemq.artemis.core.config.impl
where also its test resides.
It will break API on integration? you could move it and keep an
extension deprecated?
This move should also not break the API I just move from one impl to
another.
Christian
--
Christian Schneider
http://www.liquid-reality.de
Open Source Architect
http://www.talend.com
