activemq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Christian Schneider <ch...@die-schneider.net>
Subject Re: Refactoring of org.apache.activemq.artemis.utils
Date Tue, 17 Nov 2015 10:05:16 GMT
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
> <chris@die-schneider.net> 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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message