Cargo

Decouple JVM launching from Ant's Java task and allow clients to provide their own JVM launcher

Details

  • Type: New Feature New Feature
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: None
  • Fix Version/s: 1.1.0
  • Component/s: Core
  • Labels:
    None
  • Complexity:
    Intermediate
  • Patch Submitted:
    Yes
  • Number of attachments :
    3

Description

The motivating use case: I would like to use Cargo to start containers from Eclipse. More importantly, I would like to attach the Eclipse debugger to the container/server JVM started by Cargo, to debug the servlets being deployed there. Technically, this boils down to use IVMRunner from the Eclipse JDT APIs. However, the JVM launching code in Cargo is currently hard-wired to use Ant's Java task.

In essence, the attached patch introduces the interface org.codehaus.cargo.container.spi.jvm.JvmLauncher. This interface is basically an abstraction of Ant's java task, allowing to provide alternative implementations (like the one from Eclipse). Subclasses of SpawnedContainer provide a setter that allows the client code to set the desired factory for the JvmLauncher instances being used. By default, a factory wrapping the Ant java task is used.

It's worth to note that Cargo forks JVMs for various tasks: starting a server, stopping it, pinging it, deploying into it, etc. For my case, I only want to provide a custom JvmLauncher when starting the server but stick with the Ant/default impl for all other JVMs. To enable this decision making, the JvmLauncherFactory is provided with a boolean to indicate whether a server JVM or some other client/utility JVM is launched. Instead of a boolean, an enum could have been used to denote the purpose of the launched JVM but I didn't see concrete use cases for such a fine-grained distinction so I went with the simple boolean.

This abstraction constitues a breaking change to the container SPI (which I hope is worth for Cargo 1.1) and the bulk of the changes in the patch deal with adjusting the existing container impls to the new SPI.

AFAICT, AntContainerExecutorThread is now only used from one package. If wanted, this class could be moved and even made package-private.

  3. Text File
    jvm-launcher.patch
    04/Mar/11 7:59 AM
    186 kB
    Benjamin Bentmann

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Benjamin Bentmann added a comment -

Revised patch, missed some ITs before. Might still have to revise later, I have troubles running some of the container ITs.

Show
Benjamin Bentmann added a comment - Revised patch, missed some ITs before. Might still have to revise later, I have troubles running some of the container ITs.
Hide
Permalink
Savas Ali Tokmen added a comment -

Hi Benjamin

This patch looks pretty massive, which container do you test with?

Thank you

Show
Savas Ali Tokmen added a comment - Hi Benjamin This patch looks pretty massive, which container do you test with? Thank you
Hide
Permalink
Benjamin Bentmann added a comment -

So far, I have only been able to successfully run the sample ITs for jo, jonas, jetty, geronimo and resin. For other containers like glassfish, jboss and tomcat I get failures even with plain cargo trunk which I hadn't had the time/motivation to look further into.

Show
Benjamin Bentmann added a comment - So far, I have only been able to successfully run the sample ITs for jo, jonas, jetty, geronimo and resin. For other containers like glassfish, jboss and tomcat I get failures even with plain cargo trunk which I hadn't had the time/motivation to look further into.
Hide
Permalink
Savas Ali Tokmen added a comment -

What kind of failures are you having on which container?

glassfish2x requires to be installed manually first, and I know tomcat4x is "randomly stable" (that's because we run it on a too new JVM I guess).

Show
Savas Ali Tokmen added a comment - What kind of failures are you having on which container? glassfish2x requires to be installed manually first, and I know tomcat4x is "randomly stable" (that's because we run it on a too new JVM I guess).
Hide
Permalink
Benjamin Bentmann added a comment -

I managed to get a good glassfish run on trunk, jboss and tomcat fails with the attached errors.

Show
Benjamin Bentmann added a comment - I managed to get a good glassfish run on trunk, jboss and tomcat fails with the attached errors.
Hide
Permalink
Benjamin Bentmann added a comment -

Updated patch, the refactoring broke AbstractGlassFishInstalledLocalContainer.invokeAsAdmin() to not use the configured JAVA_HOME when forking the command.

On a related page, there are four other invocations of createAntTask("java") in the original code (Geronimo, Jonas) that don't obey the configured java home property AFAICT. But that's something for a future refactoring.

Show
Benjamin Bentmann added a comment - Updated patch, the refactoring broke AbstractGlassFishInstalledLocalContainer.invokeAsAdmin() to not use the configured JAVA_HOME when forking the command. On a related page, there are four other invocations of createAntTask("java") in the original code (Geronimo, Jonas) that don't obey the configured java home property AFAICT. But that's something for a future refactoring.
Hide
Permalink
Savas Ali Tokmen added a comment -

I've taken a closer look in your logs at what is failing:

  • On Tomcat, only Tomcat 4.x is failing and that probably is our "random" issues. I think this can be ignored.
  • On JBoss, the remote deployer for 5.1.x and 6.x is failing with a NPE. Did you change any JBossDeployer class?
Show
Savas Ali Tokmen added a comment - I've taken a closer look in your logs at what is failing: On Tomcat, only Tomcat 4.x is failing and that probably is our "random" issues. I think this can be ignored. On JBoss, the remote deployer for 5.1.x and 6.x is failing with a NPE. Did you change any JBossDeployer class?
Hide
Permalink
Benjamin Bentmann added a comment -

The test failures were all generated with Cargo trunk, i.e. without my patch to try and get a baseline.

Show
Benjamin Bentmann added a comment - The test failures were all generated with Cargo trunk, i.e. without my patch to try and get a baseline.
Hide
Permalink
Benjamin Bentmann added a comment -

It's probably not really worth/interesting, to figure out the reasons for my test failures. What matters is actually whether my patch breaks things. So if you have a good baseline and observe new/unexpected test failures after applying my patch, please let me know what fails such that I can review the relevant refactoring of the container.

Show
Benjamin Bentmann added a comment - It's probably not really worth/interesting, to figure out the reasons for my test failures. What matters is actually whether my patch breaks things. So if you have a good baseline and observe new/unexpected test failures after applying my patch, please let me know what fails such that I can review the relevant refactoring of the container.
Hide
Permalink
Savas Ali Tokmen added a comment -

You still don't want to check in the code yourself, do you?

Show
Savas Ali Tokmen added a comment - You still don't want to check in the code yourself, do you?
Hide
Permalink
Savas Ali Tokmen added a comment -

bentmann added as cargo Developer

Show
Savas Ali Tokmen added a comment - bentmann added as cargo Developer
Hide
Permalink
Savas Ali Tokmen added a comment -

Looks good to me ...

Hopefully, all tests and checkstyle and everything are passing aren't they?

Show
Savas Ali Tokmen added a comment - Looks good to me ... Hopefully, all tests and checkstyle and everything are passing aren't they?
Hide
Permalink
Benjamin Bentmann added a comment -

Applied in r2721.

Show
Benjamin Bentmann added a comment - Applied in r2721 .

People

  • Assignee:
    Benjamin Bentmann
    Reporter:
    Benjamin Bentmann
Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: