geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "gianny DAMOUR" <gianny_dam...@hotmail.com>
Subject Re: Review of refactored deployment classes
Date Wed, 15 Oct 2003 12:43:04 GMT
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


Mime
View raw message