Re: Implementation pluggability

> 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.
>