geronimo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
Subject [jira] Commented: (GERONIMO-106) Deployment Planner base-class
Date Thu, 23 Oct 2003 00:36:46 GMT
The following comment has been added to this issue:

     Author: Gianny DAMOUR
    Created: Wed, 22 Oct 2003 7:36 PM
Hello Aaron,

> I don't agree with the patch as it stands, for a number of reasons.  However,
> there is good code here, and I think I can rearrange it into a more agreeable
> form.
Thanks for that.

>  - Don't like the packaging (both actual names and decisions on which classes go
> where)
I agree. I was unable to decide where to put the classes. Indeed, by now there are three packages
related to deployment capabilities:

- org.apache.geronimo.deployment;
- org.apache.geronimo.enterprise.deploy; and
- org.apache.geronimo.xml.deployment.

Could you please shade some light on why one does not have a simple:

- org.apache.geronimo.deployment.

package containing sub-packages?

>  - Don't think DDLoader merits 3 classes, particularly when the caller needs to
> to cast the result anyway, and on top of that it hardcodes the module type to
> loader class mapping
For this one, I disagree. This is a standard pattern: Abstract factory + factory builder.
DDLoaderImpl, which hardcodes the mapping, is "fine" based on this pattern.

A simple comparaison with the standard XML API:

- DocumentBuilderFactory <-> DDLoaderFactory;
- DocumentBuilder <-> DDLoader;

The difference in my approach is that I have defined an interface for DDLoader and not an
abstract class.

> - Don't see why DeploymentContext and DeploymentMetaData are different
I think that DeploymentMetaData is OK only for standard J2EE module deployments. However,
I assume that the meta-data repository for a service does not define the same attributes.
Hence these 2 classes. I plan to migrate ServiceDeploymentPlanner when this patch will be
approved. My idea is that the method createMetaDataMBean should return a DeploymentContext
and not a DeploymentMetaData instance. I will patch ModuleDeploymentSupport in order to reflect
this idea during the migration of ServiceDeploymentPlanner.

> - Don't see why the change to AbstractManagedObject is in here
I agree. This one is rather strange... My bad. As you know, AbsractContainer is an AbstractStateManageable
and not an AbstractManagedObject.

AFAIK, Greg Wilkins will fix this: AbsractContainer should be an AbstractManagedObject and
AbstractStateManageable should be removed.

AbstractStateManageable defines the relation service and the dependency service as protected.
AbstractManagedObject defines them as private. Because, ModuleDeploymentSupport needs these
instances and because the current hierarchy is wrong, I have decided to patch AbstractManagedObject
at the same time.

I agree this is weird.

View the issue:

Here is an overview of the issue:
        Key: GERONIMO-106
    Summary: Deployment Planner base-class
       Type: Improvement

     Status: Assigned
   Priority: Major

 Time Spent: Unknown
  Remaining: Unknown

    Project: Apache Geronimo

   Assignee: Aaron Mulder
   Reporter: Gianny DAMOUR

    Created: Tue, 21 Oct 2003 9:55 AM
    Updated: Tue, 21 Oct 2003 10:00 AM

This patch is an enhancement of GERONIMO-102, which was a "sample code" used to progress a
discussion about the responsibities of Deployment Planners.

It should fix most of the drawbacks identified by Jan Bartel. Some points have not been fixed.
Indeed, some of them require a significant impact of the code-base.

This message is automatically generated by JIRA.

If you think it was sent incorrectly contact one of the administrators:

If you want more information on JIRA, or have a bug to report see:

View raw message