MainDeployerImpl.deploy and checkComplete

[alesj]

With the full impl of JBMICROCONT-187 (which is done :-)), this needs changing in MainDeployerImpl.deploy:

 public void deploy(Deployment deployment) throws DeploymentException
 {
 addDeployment(deployment);
 process();
 // FIXME don't check contexts
 checkComplete(deployment);
 }

Since it's completely legit to have not yet fully installed contexts - depending beans, OSGi deployments, ...

Adding 'checkStructureComplete(Deployment)' in DeployerClient --> only checking if deployment is in error or missing deployer?

[adrian.brock]

"alesj" wrote:
With the full impl of JBMICROCONT-187 (which is done :-)), this needs changing in MainDeployerImpl.deploy:

 public void deploy(Deployment deployment) throws DeploymentException
 {
 addDeployment(deployment);
 process();
 // FIXME don't check contexts
 checkComplete(deployment);
 }

Since it's completely legit to have not yet fully installed contexts - depending beans, OSGi deployments, ...

Adding 'checkStructureComplete(Deployment)' in DeployerClient --> only checking if deployment is in error or missing deployer?

No that's not the purpose of the single deployment api.
It is a short hand way of doing:

deployerClient.addDeployment(deployment);
deployerClient.process();
deployerClient.checkComplete();

Except in a thread safe way. In fact the code should be:

 public void deploy(Deployment deployment) throws DeploymentException
 {
 addDeployment(deployment);
 process(deployment);
 try
 {
 checkComplete(deployment);
 }
 catch (DeploymentException e)
 {
 // Didn't work, roll it back
 removeDeployment(deployment);
 process(deployment);
 throw e;
 }
 }

Where process(deployment) only processes the passed deployment,
not other deployments that may have been added on other threads.

But more importantly, it should really be an atomic operation.
i.e. the deployment is either fully working or it is not deployed at all.

If you want to add a less strict convience api that is fine,
but it is a 5% use case not the normal 95% use case of this method.
It's not related to this issue.

[alesj]

So, MainDeployerImpl.deploy was always meant to be all or nothing?
Why are then your tests (MockClassLoaderDependenciesUnitTestCase) not relying on this? :-)
I got this half-check notion from their failure. ;-)

[adrian.brock]

"alesj" wrote:
So, MainDeployerImpl.deploy was always meant to be all or nothing?
Why are then your tests (MockClassLoaderDependenciesUnitTestCase) not relying on this? :-)
I got this half-check notion from their failure. ;-)

That's the problem with writing tests against a incomplete and not properly implemented api.

The compiler should have an
@Incomplete annotation to go with @Deprecated
to warn you that you are probably writing against an api that will change! ;-)

[alesj]

Regarding the single deployment process api.
Is adding proper locks (similar to what we do in AbstractController) the way to go, or should just the relevant methods be re-written to support single deployment in correct way with regard to topLevelDeployments, allDeployments, ... collections in MainDeployerImpl?

[adrian.brock]

Actually, thinking about it the signatures should really be:

 void deploy(Deployment... deployment) throws DeploymentException
 void undeploy(Deployment... deployment) throws DeploymentException

So you can deploy multiple deployment together in a thread safe
and atomic way.

This will make the implementation a bit more complicated, buit it will be
more useful when you need to deploy mutiple deployments that are related.
This happens a lot in the JBossAS testsuite.
e.g.

getDeploySetup(MyTestCase.class, "queues-service.xml, test.ear");

The ... means it will also work for a single deployment as well.

[adrian.brock]

"alesj" wrote:
Regarding the single deployment process api.
Is adding proper locks (similar to what we do in AbstractController) the way to go, or should just the relevant methods be re-written to support single deployment in correct way with regard to topLevelDeployments, allDeployments, ... collections in MainDeployerImpl?

The existing process() should be re-written to prebuild the list of deployments
then invoke a new process(Deployment... deployments).