Re: Implementation pluggability

> Kohsuke Kawaguchi wrote:
> > Matt Wringe wrote:
> >> On Fri, 2008-07-11 at 10:17 -0700, Kohsuke Kawaguchi wrote:
> >>> Does this direction make sense?
> >>>
> >>> I guess what I'm trying to avoid here is to spend time implementing this
> >>> feature, only to learn later that you don't agree with the direction.
> >>>
> >> it makes sense to me :)
> >
> > Attached is the patch for this change that covers Deployable and
> > DeployableFactory. If this looks OK, I'll proceed with all the other
> > factory types.
> >
> > See the test code for how the container implementation would look like.
> >
> >
> >>> Kohsuke Kawaguchi wrote:
> >>> > Matt Wringe wrote:
> >>> >> On Tue, 2008-07-08 at 11:28 -0700, Kohsuke Kawaguchi wrote:
> >>> >>> Matt Wringe wrote:
> >>> >>> > On Mon, 2008-07-07 at 18:25 -0700, Kohsuke Kawaguchi wrote:
> >>> >>> >> I just filed http://jira.codehaus.org/browse/CARGO-584 but I wanted to
> >>> >>> >> check with the team if people think this makes sense (hopefully they would.)
> >>> >>> >
> >>> >>> > Replied to the jira about how cargo already supports custom containers
> >>> >>> > and configuration.
> >>> >>>
> >>> >>> Thanks. I added my comment there, too. In short, I think what you showed
> >>> >>> me is a good feature, but IMHO it's not suffice.
> >>> >>>
> >>> >>>
> >>> >>> >> The implementation approach that I'm thinking of is to have
> >>> >>> >> DefaultXXXFactory discover other XXXFactory implementations through
> >>> >>> >> META-INF/services and use them as delegates on methods like
> >>> >>> >> "getDeployerClass". So the container implementations that would like to
> >>> >>> >> hook into this would implement their own factories and write
> >>> >>> >> META-INF/services entry.
> >>> >>> >
> >>> >>> > I do not have a lot of experience with jar service providers so I don't
> >>> >>> > really know how feasible this is.
> >>> >>>
> >>> >>> I do have a lot of experience, so FWIW, rest assured that it will work.
> >>> >> ok :) could you explain a bit more how exactly it would work then?
> >>> >
> >>> > This is a standard convention used widely everywhere, such as in many
> >>> > JCP APIs.
> >>> >
> >>> > The technique is for the use cases where typically one wants to discover
> >>> > implementations of some contract defined in the API. For example, in
> >>> > JAXP, SAXParserFactory is the API, and the API discovers vendor-specific
> >>> > implementations (like Xerces) without having any prior knowledge about
> >>> > such an implementation.
> >>> >
> >>> > Here is the technique. The base contract is defined in terms of a Java
> >>> > class/interface. Suppose this contract has the fully-qualified name of
> >>> > org.codehaus.cargo.generic.deployer.DeployerFactory. Implementations
> >>> > that like to be discovered will put a file with the name
> >>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
> >>> > in the jar file. This file is UTF-8 encoded, and it lists fully
> >>> > qualified names of classes that implement this base interface/class.
> >>> > Let's say I write "cargo-glassfish-impl.jar". I put this file in my jar,
> >>> > and its content may read "org.glassfish.cargo.DeployerFactoryImpl"
> >>> >
> >>> > The discovery process takes one ClassLoader as a parameter. This
> >>> > parameter is normally defaulted to
> >>> > Thread.currentThread().getContextClassLoader(), but in more complex
> >>> > scenarios the caller do need to be able to pass it in.
> >>> >
> >>> > The API then lists up all
> >>> > "META-INF/services/org.codehaus.cargo.generic.deployer.DeployerFactory"
> >>> > from resources, parse them all, and list up all the classes. It'll
> >>> > instantiate them and use them, with suitable error handling in-between.
> >>> >
> >>> > commons-discovery defines code to do this. Writing it from scratch is
> >>> > also rather easy.
> >>> >
> >>> > The end result is that all that needed to be done by the user is put
> >>> > "cargo-glassfish-impl.jar" in the classpath.
> >>> >
> >>> >
> >>> >>> > I really don't like the current factory implementation and would ideally
> >>> >>> > like to see cargo automatically determine the proper classes to use
> >>> >>> > based on a class naming scheme (and cargo is already using a naming
> >>> >>> > scheme, ie Tomcat6xInstalledLocalContainer is the container
> >>> >>> > implementation for Tomcat6 for a local installed container).
> >>> >>> >
> >>> >>> > With the current factory implementation its too easy to make
> >>> >>> mistakes.
> >>> >>>
> >>> >>> I guess the only problem with the naming convention is that all the
> >>> >>> implementations would have to live in the "org.codehaus" package.
> >>> >>
> >>> >> well, it could just look at the class name and not the package name.
> >>> >
> >>> > OK, so your idea of pluggability must be bit different than mine. I
> >>> > suspect your approach is that somehow container implementations would
> >>> > tell the factory about its class names, and the factory would figure out
> >>> > the ID and the type from the class name. But this is missing how the
> >>> > containers tell the factory their class names in the first place,
> >>> > without asking the users' help --- that is the part that I'm trying to
> >>> > eliminate.
> >>> >
> >>> > What I was thinking was that the factory implementation now only know
> >>> > about a class name convention, and whenever someone requests an
> >>> > instantiation from some parameters, it just blindly tries to create a
> >>> > class of certain names. If the class is there, it works, otherwise, it
> >>> > won't.
> >>> >
> >>> > So in the latter scheme, while you can try more than one class names,
> >>> > you cannot really allow arbitrary package names or class names, simply
> >>> > because the factory can't know about them all.
> >>> >
> >>> >
> >>> >>> But if the Cargo project is OK for random people to be putting classes
> >>> >>> in this package,
> >>> >>
> >>> >> I don't think it would be ok for people to be using
> >>> >> org.codehaus.cargo.... packages as it would give the false impression on
> >>> >> the code origins. Please see my comment above.
> >>> >
> >>> > OK. Then that eliminates convention-only approach.
> >>> >
> >>> >
> >>> >>>  this is probably better than META-INF/services look up,
> >>> >>> because the container implementor needs to write less.
> >>> >>>
> >>> >>> Oh, and you'd have to rename some of the existing classes, I think, like
> >>> >>> JBossXXX, because it would have inconsistent case convention. And the
> >>> >>> naming convention would be somewhat contrived to make it fit with the
> >>> >>> current class names.
> >>> >>
> >>> >> I am not entirely sure how it would work, I was just throwing out ideas.
> >>> >
> >>> > OK.
> >>> >
> >>> >
> >>> >>> >> I feel container developers would have to write more than they should,
> >>> >>> >> but OTOH the deployer/container/etc uses tuples for keys, so doing
> >>> >>> >> anything simpler means introducing another factory-like mechanism, which
> >>> >>> >> I hesitate.
> >>> >>> >>
> >>> >>> >>
> >>> >>> >> If it makes sense, I'll first prepare a patch for, say,
> >>> >>> >> DefaultDeployerFactory. If that looks all right, I'll do the same for
> >>> >>> >> other factories.
> >>> >>> >>
> >>> >>> >>
> >>> >>> >> I'd like to eventually resurrect the GlassFish support work,
> >>> >>> >
> >>> >>> > great, we need someone with glassfish knowledge
> >>> >>> >
> >>> >>> >>  but I think
> >>> >>> >> this should be the first step, so that various people in the GlassFish
> >>> >>> >> project can eat the dogfood before it can be considered for the
> >>> >>> >> inclusion into Cargo core.
> >>> >>> >
> >>> >>> > I wouldn't worry about trying to get a perfectly working implementation
> >>> >>> > before considering contributing to cargo. You can always start with some
> >>> >>> > simple commands and work from there. If needed we can always add
> >>> >>> > documentation about it being a feature preview.
> >>> >>>
> >>> >>> I think the feature still makes sense. Code that works is one reason,
> >>> >>> but to be honest, a bigger reason for me is the checkstyle rule. Things
> >>> >>> might have changed now, but back when I contributed the embedded Tomcat
> >>> >>> support, the checkstyle rules in Cargo project was very draconian.
> >>> >>
> >>> >> hehe, yes, I agree here.
> >>> >>
> >>> >>> It
> >>> >>> was really painful for me to make sure every check in satisfies all the
> >>> >>> check style rules.
> >>> >>
> >>> >> Its really not that bad when you use an ide to manage it for you. Right
> >>> >> now in eclipse I can basically just highlight the code and tell eclipse
> >>> >> to refactor it to fix the guidelines. And it also highlights the issues
> >>> >> and tells you what is wrong with them (although I can't seem to figure
> >>> >> out how to tell eclipse where the suppression file is).
> >>> >
> >>> > Yes, I was hoping that at least there are some IDEs that understand
> >>> > checkstyle rule files. Unfortunately, I don't think such a feature is
> >>> > available on my IDE, hence the preference to postpone till the end.
> >>> >
> >>> >
> >>> >>> By working on a separate module, I'm hoping that I can postpone doing
> >>> >>> that work until we integrate that back into the core (and perhaps at
> >>> >>> that point I can ask for other people's help.)
> >>> >>
> >>> >> You can also easily disabled it by passing a specific -D option during a
> >>> >> build.
> >>> >
> >>> > Yes. I'm aware of this, but again, the rule back then was that for me to
> >>> > make a commit, it better passes the checkstyle.
> >>> >
> >>> >> If you are really worried about the checkstyle, we could even
> >>> >> temporarily disable it for the Glassfish part.
> >>> >
> >>> > That's good to know :-)
> >>> >
> >>> > But I really think it's better for the Cargo project as a whole it it
> >>> > lets people write containers outside the Cargo project.
> >>> >
> >>> > Think about Ant tasks, for example. If Ant didn't define a way for
> >>> > people to write custom tasks outside the Apache Ant project itself, I
> >>> > don't think it would have been this successful.
> >>> >
> >>>
> >>>
> >>
> >>
> >> ---------------------------------------------------------------------
> >> To unsubscribe from this list, please visit:
> >>
> >>     http://xircles.codehaus.org/manage_email
> >>
> >>
> >>
> >
> >
>
>