EJB subsystem refactoring

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

EJB subsystem refactoring

Stuart Douglas
Hi everyone,

There is some fairly major refactoring I would like to do ti the EJB/EE subsystem, I just thought I would run it by everyone to see if anyone had any suggestions / objections.

1) Remove AbstractAnnotationEJBProcessor, and replace it with a system that attaches annotation information directly to EEModuleClassDescription 

I have a prototype of this at: 

https://github.com/stuartwdouglas/jboss-as/compare/jbossas:master...stuartwdouglas:ee-reorg

This approach has a number of advantages:

- AbstractAnnotationEJBProcessor does not handle annotations in different sub deployments properly, so if a bean inherits from classes in another sub deployment those annotations will not be processed (this has potential security implications for things like the EJB security annotations).

- This should remove a lot of boilerplate code that is present in the annotation processors (I know at the moment the patch increases the total loc slightly, but after all the processors are converted it should reduce it)

- It should give us a more consistent approach to merging, at the moment there is a deployment descriptor processor, and annotation processor, and a merging processor, that are mostly copied / pasted all over the place. With this change there will only by 1 processor, that is responsible for merging the annotation and descriptor data, and attaching it to the description/configuration. 


2) Subclass ComponentConfiguration for each component type

At the moment there is no where to store post class loading metadata for EJB components. This has been worked around with nasty hacks, either by storing post-class loading stuff in the description:


Or by performing configuration work in the ComponentCreateService that should really be done in DUP's / configurators:


In some cases this has resulted in EjbComponent's using maps keyed on MethodIdentifier internally, rather than a faster IdentityMap with the actual method objects from the reflection index. 


There are also places in the code where we go from a Method object, store a MethodIdentifer, and then later re-resolve the original Method object, which is not ideal.

Does all this sound reasonable?

Stuart

_______________________________________________
jboss-as7-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/jboss-as7-dev
Reply | Threaded
Open this post in threaded view
|

Re: EJB subsystem refactoring

Jaikiran Pai
On Monday 22 August 2011 08:42 AM, Stuart Douglas wrote:
> Hi everyone,
>
> There is some fairly major refactoring I would like to do ti the
> EJB/EE subsystem, I just thought I would run it by everyone to see if
> anyone had any suggestions / objections.
>
> 1) Remove AbstractAnnotationEJBProcessor, and replace it with a system
> that attaches annotation information directly to EEModuleClassDescription
Yes, I would like that one to go too. I had removed a few usages to
that, when we ran into the issues that you mention, but there are some
more DUPs which are still using it.

>
>
> This approach has a number of advantages:
>
> ...
> - It should give us a more consistent approach to merging, at the
> moment there is a deployment descriptor processor, and annotation
> processor, and a merging processor, that are mostly copied / pasted
> all over the place. With this change there will only by 1 processor,
> that is responsible for merging the annotation and descriptor data,
> and attaching it to the description/configuration.
>
Is this specific to just EJB3? Because we need similar construct for
other parts too (whichever component requires annotation, deployment
descriptor and merging).
>
> 2) Subclass ComponentConfiguration for each component type
>
> At the moment there is no where to store post class loading metadata
> for EJB components. This has been worked around with nasty hacks,
> either by storing post-class loading stuff in the description:
>
When we started with refactoring EE some time back, I remember David had
mentioned a reason why he didn't want the ComponentConfiguration to be
sub-classed. I no longer remember what that reason was. But yeah, you
are right that at present, we end up pushing such data to the
(sub-classed) component create service and pass it on to the component.
> Does all this sound reasonable?
>
>
Overall, I don't have any objection to these changes.

-Jaikiran

_______________________________________________
jboss-as7-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/jboss-as7-dev
Reply | Threaded
Open this post in threaded view
|

Re: EJB subsystem refactoring

Carlo de Wolf
On 08/22/2011 08:40 AM, Jaikiran Pai wrote:

> On Monday 22 August 2011 08:42 AM, Stuart Douglas wrote:
>> Hi everyone,
>>
>> There is some fairly major refactoring I would like to do ti the
>> EJB/EE subsystem, I just thought I would run it by everyone to see if
>> anyone had any suggestions / objections.
>>
>> 1) Remove AbstractAnnotationEJBProcessor, and replace it with a
>> system that attaches annotation information directly to
>> EEModuleClassDescription
> Yes, I would like that one to go too. I had removed a few usages to
> that, when we ran into the issues that you mention, but there are some
> more DUPs which are still using it.
+1

It needs to be replaced by something that mimics the way we operate. It
was an outgrowth of duplicate code that popped up in the DUPs.

First and foremost we need to decide whether the DUPs are going to be
target metadata driven. Or do we still want to separate different stages
like annotation processing, parsing, merging etc?

>>
>>
>> This approach has a number of advantages:
>>
>> ...
>> - It should give us a more consistent approach to merging, at the
>> moment there is a deployment descriptor processor, and annotation
>> processor, and a merging processor, that are mostly copied / pasted
>> all over the place. With this change there will only by 1 processor,
>> that is responsible for merging the annotation and descriptor data,
>> and attaching it to the description/configuration.
>>
> Is this specific to just EJB3? Because we need similar construct for
> other parts too (whichever component requires annotation, deployment
> descriptor and merging).
>>
>> 2) Subclass ComponentConfiguration for each component type
>>
>> At the moment there is no where to store post class loading metadata
>> for EJB components. This has been worked around with nasty hacks,
>> either by storing post-class loading stuff in the description:
>>
> When we started with refactoring EE some time back, I remember David
> had mentioned a reason why he didn't want the ComponentConfiguration
> to be sub-classed. I no longer remember what that reason was. But
> yeah, you are right that at present, we end up pushing such data to
> the (sub-classed) component create service and pass it on to the
> component.
I designed the effigy API specifically for a post-classloader metadata
store. So why not use it?

In fact we had some code in the past where parts of the component
description/configuration acted as effigy facades.

Most importantly you want to separate the runtime from any of the
description/configuration hacks.

Carlo
>> Does all this sound reasonable?
>>
>>
> Overall, I don't have any objection to these changes.
>
> -Jaikiran
>

_______________________________________________
jboss-as7-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/jboss-as7-dev
Reply | Threaded
Open this post in threaded view
|

Re: EJB subsystem refactoring

Stuart Douglas

On 22/08/2011, at 6:11 PM, Carlo de Wolf wrote:

> On 08/22/2011 08:40 AM, Jaikiran Pai wrote:
>> On Monday 22 August 2011 08:42 AM, Stuart Douglas wrote:
>>> Hi everyone,
>>>
>>> There is some fairly major refactoring I would like to do ti the EJB/EE subsystem, I just thought I would run it by everyone to see if anyone had any suggestions / objections.
>>>
>>> 1) Remove AbstractAnnotationEJBProcessor, and replace it with a system that attaches annotation information directly to EEModuleClassDescription
>> Yes, I would like that one to go too. I had removed a few usages to that, when we ran into the issues that you mention, but there are some more DUPs which are still using it.
> +1
>
> It needs to be replaced by something that mimics the way we operate. It was an outgrowth of duplicate code that popped up in the DUPs.
>
> First and foremost we need to decide whether the DUPs are going to be target metadata driven. Or do we still want to separate different stages like annotation processing, parsing, merging etc?

In my prototype I have a two phase process, first the annotations are read into the shared EEModuleClassDescription, and then later they are merged with the DD data into the description.


>>>
>>>
>>> This approach has a number of advantages:
>>>
>>> ...
>>> - It should give us a more consistent approach to merging, at the moment there is a deployment descriptor processor, and annotation processor, and a merging processor, that are mostly copied / pasted all over the place. With this change there will only by 1 processor, that is responsible for merging the annotation and descriptor data, and attaching it to the description/configuration.
>>>
>> Is this specific to just EJB3? Because we need similar construct for other parts too (whichever component requires annotation, deployment descriptor and merging).
>>>
>>> 2) Subclass ComponentConfiguration for each component type
>>>
>>> At the moment there is no where to store post class loading metadata for EJB components. This has been worked around with nasty hacks, either by storing post-class loading stuff in the description:
>>>
>> When we started with refactoring EE some time back, I remember David had mentioned a reason why he didn't want the ComponentConfiguration to be sub-classed. I no longer remember what that reason was. But yeah, you are right that at present, we end up pushing such data to the (sub-classed) component create service and pass it on to the component.
> I designed the effigy API specifically for a post-classloader metadata store. So why not use it?
>
> In fact we had some code in the past where parts of the component description/configuration acted as effigy facades.
>
> Most importantly you want to separate the runtime from any of the description/configuration hacks.

I'm not really sure what the Effigy API can do for us here, it looks like it is not really a good fit for the current description / configuration approach.

Stuart

>
> Carlo
>>> Does all this sound reasonable?
>>>
>>>
>> Overall, I don't have any objection to these changes.
>>
>> -Jaikiran
>>
>


_______________________________________________
jboss-as7-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/jboss-as7-dev
Reply | Threaded
Open this post in threaded view
|

Re: EJB subsystem refactoring

Carlo de Wolf
On 08/22/2011 11:44 AM, Stuart Douglas wrote:

> On 22/08/2011, at 6:11 PM, Carlo de Wolf wrote:
>
>> On 08/22/2011 08:40 AM, Jaikiran Pai wrote:
>>> On Monday 22 August 2011 08:42 AM, Stuart Douglas wrote:
>>>> Hi everyone,
>>>>
>>>> There is some fairly major refactoring I would like to do ti the EJB/EE subsystem, I just thought I would run it by everyone to see if anyone had any suggestions / objections.
>>>>
>>>> 1) Remove AbstractAnnotationEJBProcessor, and replace it with a system that attaches annotation information directly to EEModuleClassDescription
>>> Yes, I would like that one to go too. I had removed a few usages to that, when we ran into the issues that you mention, but there are some more DUPs which are still using it.
>> +1
>>
>> It needs to be replaced by something that mimics the way we operate. It was an outgrowth of duplicate code that popped up in the DUPs.
>>
>> First and foremost we need to decide whether the DUPs are going to be target metadata driven. Or do we still want to separate different stages like annotation processing, parsing, merging etc?
> In my prototype I have a two phase process, first the annotations are read into the shared EEModuleClassDescription, and then later they are merged with the DD data into the description.
>
>
>>>>
>>>> This approach has a number of advantages:
>>>>
>>>> ...
>>>> - It should give us a more consistent approach to merging, at the moment there is a deployment descriptor processor, and annotation processor, and a merging processor, that are mostly copied / pasted all over the place. With this change there will only by 1 processor, that is responsible for merging the annotation and descriptor data, and attaching it to the description/configuration.
>>>>
>>> Is this specific to just EJB3? Because we need similar construct for other parts too (whichever component requires annotation, deployment descriptor and merging).
>>>> 2) Subclass ComponentConfiguration for each component type
>>>>
>>>> At the moment there is no where to store post class loading metadata for EJB components. This has been worked around with nasty hacks, either by storing post-class loading stuff in the description:
>>>>
>>> When we started with refactoring EE some time back, I remember David had mentioned a reason why he didn't want the ComponentConfiguration to be sub-classed. I no longer remember what that reason was. But yeah, you are right that at present, we end up pushing such data to the (sub-classed) component create service and pass it on to the component.
>> I designed the effigy API specifically for a post-classloader metadata store. So why not use it?
>>
>> In fact we had some code in the past where parts of the component description/configuration acted as effigy facades.
>>
>> Most importantly you want to separate the runtime from any of the description/configuration hacks.
> I'm not really sure what the Effigy API can do for us here, it looks like it is not really a good fit for the current description / configuration approach.
>
> Stuart
The configuration should once more be an effigy facade. Whether that is
really implemented as such, or just a design philosophy doesn't matter
much to me.

Carlo
>> Carlo
>>>> Does all this sound reasonable?
>>>>
>>>>
>>> Overall, I don't have any objection to these changes.
>>>
>>> -Jaikiran
>>>

_______________________________________________
jboss-as7-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/jboss-as7-dev