Gianny,
The code for factoring out deployment to a common base class is looking pretty good. I think it is pretty much in line with what I was thinking of. Let's see what Aaron has to say. Here's my more detailed comments:
General: + inconsistent use of "Dd" and "DD" when referring to Deployment Descriptor in method names (I think these should be DD as these are 2 separate words) + similarly, "pojo" in method names should be all caps + inconsistent use of "Metadata" and "MetaData" in method names + some classes have all methods synchronized, is this definitely necessary? + some of the spelling in the comments could do with a bit of a spell check (only in George Bush's lexicon is "plannification" a word :-) )
Specifics: + DeploymentHelper.java Two comments on this class: 1. as the main purpose of this class seems to be to act as a factory for Object Names associated with deployables, the name should reflect this. 2. this class might not really be necessary, as the methods might be better off as part of the ModuleDeploymentSupport class, where they can be easily overridden by subclasses, if necessary.
+ ModuleDeploymentSupport.java Good to see that this class mandates that deployers are containers. I think that was the original intention in geronimo, but that seemed to have been lost somewhere along the line.
In the planDeploy() method, goals.remove(goal) should be called only just before return, otherwise it may be removed even though subsequent code throws an exception preventing the deployment from going ahead.
The canBePlanned() method should be protected or public to allow subclasses to override it.
I'm not sure that this call should be in there:
deploymentPlan.addTask(new
StartRecursiveMBeanInstance(server,
dmdMetaData));because if this is a hot-deployment this is fine, but if this is a JSR-88 distribute () then we don't want to automatically start anything. I believe that the deployment mechanism we have now should serve both hot-deploys and distribute()s, so it would be some other code in the deployment mechanism that would be responsible for working out if it is a hot deploy and ensuring startRecursive is called on the "geronimo.deployment:role=DeploymentUnit,..." mbean.
What's the reason behind changing from having 2 DeploymentPlans: one for the deployment itself and one for all of the actual things to be deployed, a la the ServiceDeploymentPlanner? Do you know why it uses 2 instead of 1?
DeploymentMetaData.java and DeploymentContext.java:
- why do we need 2 classes? Why isn't this just one merged class?
- I'd rather see the whole POJO loader stuff hidden rather than
made explicit (as in passing Class instances of various loaders
as method/constructor params). It should be possible to throw a
URL of a deployment descriptor at a POJO factory loader class static
method and
have it:
1. convert xml->DOM
2. worry about which concrete loader to call in order to
parse the DD
All we care about is that we get a POJO out, we don't want
to get too involved in the mechanism of producing it (so
we're decoupled in case the mechanism changes).I really do hope that a common base class for all geronimo DD pojos is forthcoming .....
My other comment on this, is that I don't think it should just
be applicable to J2EE deployments. I think the existing ServiceDeploymentPlanner should be brought into line with a general deployment mechanism such as this. After all, the service is very like a J2EE module:
- it is a package (either dir or packed jar)
- it has a special classloader hierarchy
- it has a deployment descriptor (ie the xyz-service.xml file)
Therefore, some of the naming of the methods and fields which emphasise J2EE should be renamed to something more generic.
Thanks for spec'ing this code out Gianny. Aaron, what's your perspective?
cheers, Jan
