Re: Need patch review.. Builders to use DeployableModule Interface

[Tim McConnell <ti...@gmail.com>]

Hi Sachin, +1, but I have a few minor comments, questions, and concerns below:

--- I really like the way you've allowed for incremental changes by extending the 
ModuleBuilder and ConfigurationBuilder interfaces. It would have been quite a daunting 
chore otherwise. Very well done.

--- I notice that the ModuleBuilder2.java file is not included in the patch but I got a 
sense of how it would look by comparing ConfigurationBuilder and ConfigurationBuilder2

--- Just FYI, there have been some directory name changes since you created your patch 
(i.e., jetty-->jetty6 and tomcat-->tomcat6)

--- I notice there is no traceability for these changes--not sure if this is frowned upon 
in open source, but it seems like from a maintenance perspective it might be useful to 
have the JIRA referenced in the comments (e.g., GERONIMO-1526 or G-1526) to be able to 
quickly isolate all the related changes.

--- I also notice that ConnectorModuleBuilder.java has very similar--not exact, but 
similar signatures for the createModule and installModule methods. For example below. My 
only concern is that if any of the test code tries to invoke one of these methods passing 
a null for either DeployableModule or JarFile, the  compiler will not be able to resolve 
which method to use and a compile error will ensue. This might not be a problem since you 
can obviously build will this  new code, but it might be for some of the other builders as 
more changes of this type are rolled out. Is it fair to assume that the signatures using 
JarFile will eventually be removed when all the code is migrated ?? If so, it's even less 
of a concern to me.

     public Module createModule(File plan, DeployableModule deployable, Naming naming, 
ModuleIDBuilder idBuilder)
     public Module createModule(File plan, JarFile moduleFile, Naming naming, 
ModuleIDBuilder idBuilder)

--- There are some comments with "TODO Geronimo-1526" in them but most appear to have been 
resolved already

--- There is some inconsistency in the usage of the interface's cleanup() method. Some 
check for null before all it and some do not. I wonder if there should be a best-practice 
failsafe to check for null in the implementation of the interface (e.g., JarDeployable) ?? 
Otherwise a nullpointerexception will occur if the deployable module is in fact null

--- DeploymentUtil.java imports DeployableModule and JarDeployable but does reference them

--- EARConfigbuilder.java lines 281 through 284 confused me for a minute but I think it 
might just be the formatting. To me it appears that the if statement at line 282 would 
apply to the jarfile that was just instantiated in 281, but I think not. It might make 
more sense though to move line 281 after the check for Instance of JarDeployable since if 
it's not a Jar file we're going to get an exception anyway.

--- That's it--I'm anxious for this code to get into 2.0 though since I have to start 
making JSR-88 changes for annotations in many of these same classes and the fewer 
conflicts the better.

Thanks,
Tim

Sachin Patel wrote:
> I've attached a patch 
> http://issues.apache.org/jira/browse/GERONIMO-2638 that provides the 
> changes from our builders use of JarFile to a new interface 
> DeployableModule.  Since 1.2 is currently less disruptive, I've created 
> a patch that can be applied to 1.2 and eventually I'll port it to 
> trunk.  I did the changes in a way so that all the builders don't have 
> to change at once and the migration can be done incrementally.  Thus the 
> use of the "temporary" classes, ModuleBuilder2, ConfigurationBuilder2, 
> and their abstract impls.  The current patch only changes the 
> EARConfigBuilder and the WebModuleBuilders (minus WebServices).  I want 
> to go ahead and get this out so I can get input prior to migrating the 
> rest of the builders.  So please provide your comments, concerns, feedback.
> 
> In a nutshell, the basic flow behind this change is the following..
> 
> (1) A deployableModuleType (gbean) can be set now in the 
> DeploymentManager by the client (eclipse for example)
> (2) Given the artifact to deploy, (previously a JarFile), a new 
> DeployableModule instance will be created through the 
> DeployableModuleFactory class, passing the the artifact, where its a 
> JarFile or in the eclipse case, an xml file describing the application 
> and where all of its resources, jars, classes reside.
> (3) Now the builders will now be passed around this instance of 
> DeployableModule, rather then the JarFile and the builders will process 
> the artifact, when possible, using the DeployableModule interface.
> 
> And basically thats it.  This allows Geronimo to build the configuration 
> from any directory structure, and no longer has to conform to a J2EE 
> archive structure and the copy of a resource in an IDE is the copy that 
> is running.  It is completley pluggable and you can easily provide 
> support for an IDE or even a Maven project structure.
> 
> The only thing I don't like is when you're dealing with a JarFile (the 
> primary case outside of an IDE).  I'm creating a wrapper around the Jar 
> using the DeployableModule implementation JarDeployable.  The problem is 
> that whenever you're processing the JarEntries, you can't use the 
> interface and have to call..
> 
> if(deployable.isArchive()) {
> JarFile jar = deployable.getJarFile()
> //process the jar
> } else {
> //use the interface
> }
> 
> There are only a few cases where we're travesing through the entries of 
> a Jar, so I'm not sure if people think this is a big deal or a huge 
> annoyance having to deal with 2 cases.
> 
> Please review as it would be nice to get this into M1.  If you have any 
> suggestions on how we can improve the interface, I would love to hear 
> your input as I just threw this interface together a while ago and I'm 
> sure we can improve on it.
> 
> Thanks.
> 
> -sachin
> 
> 

-- 
Thanks,
Tim McConnell