geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Tim McConnell <tim.mcco...@gmail.com>
Subject Re: GERONIMO-2816 patch
Date Mon, 12 Feb 2007 13:14:20 GMT
Great, I'll work on these changes today. Thanks much

David Blevins wrote:
> 
> On Feb 11, 2007, at 6:12 PM, Tim McConnell wrote:
> 
>> HI Dain, thanks for reviewing. Yes, you're right--I am scanning all 
>> the classes in the web module for annotations, which might be 
>> excessive. I'll take a look at the OpenEJB AnnotationDeployer and 
>> DeploymentLoader classes tomorrow and see if I can discern the 
>> technique you mention (and then try to emulate it). If I have 
>> questions, which is likely, I'll ask. Thanks much
> 
> It's pretty easy.  Essentially I made it so ClassFinder can be 
> constructed with a var-arg list of classes (may also support a plain 
> List, can't remember).  When that is done it only searches that set for 
> annotations and doesn't do the whole asm routine to come up with a list 
> of annotated classes.
> 
> And now that I mention it, if you were to shift to that style of 
> annotation searching you could disregard the comment I made earlier 
> about constructing the ClassFinder exactly once per codebase -- if you 
> could pretty much construct them and throw them away as you like.
> 
> As far as the patch, there's nothing in there that can't be easily fixed 
> and we're all on the same page, so I can check it in and you can work on 
> your updates or I can wait for a revised patch.  Whichever is easiest 
> for you.  Let me know.
> 
> -David
> 
>>
>> Dain Sundstrom wrote:
>>> Are you sure you must scan all classes in the web module for @EJB 
>>> annotations?  I don't think it is necessary or what you want to do.  
>>> I believe that you should only be checking specific classes for the 
>>> annotation such as all known servlets.  This is how we process 
>>> annotations for EJBs.  First we find all EJBs, either listed in the 
>>> ejb-jar.xml or found by scanning for @Stateless, @Stateful and 
>>> @MessageDriven.  Once we have the list of EJBs we check each class 
>>> for the presence of the @EJB annotation.
>>> In your case, I believe you are scanning for all classes that have 
>>> the @EJB annotation which will be a much larger number of classes 
>>> then just the servlets.  This over processing of classes can easily 
>>> lead to naming conflicts.  Also, I do not believe that servlet 2.5 
>>> has an @Servlet annotation, so I don't think you have to do any 
>>> general purpose scanning like EJB module must do, which should make 
>>> web deployment much more efficient.
>>> Other than that, the patches look good :)  One minor thing to note is 
>>> we don't use prefixes such as "m_" for variables.
>>> Good job,
>>> -dain
>>> On Feb 10, 2007, at 1:23 AM, Tim McConnell wrote:
>>>> Hi, I've attached a patch and two new classes to the GERONIMO-2816 
>>>> JIRA that provides support for the @EJB and @EJBs
>>>> annotations if anyone would like to review. The intent is to 
>>>> demonstrate a final/permanent technique that can be extended and 
>>>> used throughout Geronimo to support annotations for JSR-88. This 
>>>> patch and new code works for Tomcat and circumvents the annotation 
>>>> processing that is currently in EjbRefBuilder (which did not update 
>>>> the deployment descriptor with the discovered annotations, but only 
>>>> updated the JNDI references). I'd appreciate some feedback before I 
>>>> start propagating this technique to the other module builders to 
>>>> support the remainder of the annotations. I already have another 
>>>> subclass ready to support the @Resource annotations but I didn't 
>>>> want to include it in this example since it doesn't demonstrate 
>>>> anything different than the EJBAnnotationHelper subclass, and I'd 
>>>> like to get some feedback first on the the technique. The general 
>>>> technique, which we've discussed before, is pretty straightforward 
>>>> and is summarized here for reference:
>>>>
>>>> 1 -- Discover the annotations: I had hoped that we could centralize 
>>>> the discovery of annotations in the Deployer class prior to the 
>>>> createModule phase of deployment, but as David Blevins pointed out 
>>>> this is almost impossible. So it has to be pushed down into the 
>>>> installModule phase of deployment after the necessary classloader(s) 
>>>> and module context(s) have been established (see the changes for 
>>>> AbstractWebModuleBuilder for an example).
>>>>
>>>> 2 -- Process the annotations: This just means to update the existing 
>>>> deployment descriptor (or create a new one) with the discovered 
>>>> annotations.
>>>>
>>>> 3 -- Set metadata-complete in the deployment descriptor to prevent 
>>>> repeated processing of annotations (see the EjbRefBuilder changes 
>>>> for an example)
>>>>
>>>> 4 -- Update the deployment descriptor in the module so that it can 
>>>> flow through the remainder of the deployment process much as before 
>>>> (for JNDI naming and resolution, and to remain with module in 
>>>> support of the JSR-77 requirements for management).
>>>>
>>>>  --
>>>> Thanks,
>>>> Tim McConnell
>>
>> --Thanks,
>> Tim McConnell
>>
> 
> 

-- 
Thanks,
Tim McConnell

Mime
View raw message