Review of refactored deployment classes

[Jan Bartel <ja...@mortbay.com>]

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