Re: Review of refactored deployment classes

gianny DAMOUR 15 Oct 2003 12:43:16 -0000

Jan,


Thanks for your comments.

General:
+ 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 :-) )

I agree and shame on me: even in french "plannification" is miss-spelt (only one "n" in french).

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.

I agree.

+ ModuleDeploymentSupport.java

[...]

  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.

I understand. It seems that this is not an issue: it is true that the goal is removed "wrongly" from the current goal set, however this goal will be re-attempted during the next scan as no corresponding deployment MBean - MBean matching the pattern "*:role=DeploymentUnit,url=" + ObjectName.quote(url.toString()) + ",*" - is registered.

However, this is a good idea to put it just prior to return the progress status.

  The canBePlanned() method should be protected or public to allow
  subclasses to override it.

I have intentionally declared this method "private final" as it can assess all the J2EE module deployments. Perhaps that one should turn it into a template method and provide a hook to sub-classes.

   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.

This is an approach. What do you think of the following one:

When a module is distributed, it is stored in a passive directory. When this module needs to be started, one adds to the deployment scanner its URL and the standard deployment process kicks off.

To distribute a module in an auto-deploy folder will be a two step process: firstly upload it in a passive directory (one does not want a module being distributed to be retrieved by a scanner) and then move it to the auto-deploy folder. So, why not do it "in-place".

  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?

I think that to have 2 deployment plans is not "safe". If the first plan is successful and, conversely, the second one unsuccessful, then there is a deployment inconsistency: the second plan is rolled back and the first one is not.

DeploymentMetaData.java and DeploymentContext.java:
- why do we need 2 classes? Why isn't this just one merged class?

I wanted two classes for the following reason: DeploymentContext is a low-level repository focused on the files characterizing a deployment. DeploymentMetaData is a repository for J2EE module specificities (e.g. module type). I think that these two classes should be distinct as DeploymentContext could be used as a base class for other deployments. For instance, this class could be re-used by say a ServiceMetaData focusing on the specificities of a service (e.g. service DD).

- 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

I agree.

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)

I agree.

Cheers,
Gianny

_________________________________________________________________ Surf and talk on the phone at the same time with broadband Internet access. Get high-speed for as low as $29.95/month (depending on the local service providers in your area). https://broadband.msn.com

Re: Review of refactored deployment classes

Reply via email to