Precedence

[adrian.brock]

How do you define precedence rules for deployer ordering? For example, let's take a hypothetical Seam deployer that needs to augment an EJB deployment based on additional metadata within the deployment unit. It needs to happen after EJB XML and Annotation processing, but before component processing.

I liked the install/uninstall approach (see below if you dont' know what I'm talking about) because we could have added precedence metadata to the addDeployer method. Precendence metadata like, I have to come before the EJB component processing, but after the XML/annotation processing.

<install bean="MainDeployer" method="addDeployer">
 <parameter><this/></parameter>
</install>
<uninstall bean="MainDeployer" method="removeDeployer">
 <parameter><this/></parameter>
</uninstall>

See the part where I describe attachment flows.
For your example (assuming JMX components) you would do something like:

public MySeamDeployer extends AbstractSimpleRealDeployer<SeamMetaData.class>
{
 public MySeamDeployer()
 {
 // We are after everything that creates and modifies SeamMetaData
 super(SeamMetaData.class);

 // We need the EJB deployment and Annotation metadata
 setInputs(EJB3Deployment.class, AnnotationCache.class);
 // We output a -service.xml description so we are before that deployer
 setOutputs(ServiceDeploymentMetaData.class);
 }
}

[bill.burke]

 // We output a -service.xml description so we are before that deployer
 setOutputs(ServiceDeploymentMetaData.class);
 }
}

still don't see how an augmenting deployer (a deploeyr that augments a metamodel) can be sure to come before the component deployer.

If your input and output is EJB metadata, this will work?

[bill.burke]

I'm guess what I"m saying is:

setInputs(EJBMetaData.class, SeamMetaData.class);
setOutputs(EJBMetaData.class)

Will this even work?

Again, this is a deployer that modifies existing metadata. For instance, adds an interceptor because you have annotated your ejb with @org.jboss.seam.Name.

And we still have to be sure that the EJB component deployer comes after this augmentation.

[adrian.brock]

I'm guess what I"m saying is:

setInputs(EJBMetaData.class, SeamMetaData.class);
setOutputs(EJBMetaData.class)

Will this even work?

Yes. But you're welcome to examine the code and try to come up with new testcases
where it breaks. The only thing that should be not supported is loops. :-)
See DeployersImpl.sort()

The deployer above comes before anything that accepts EJBMetaData and after
anything that outputs EJBMetaData (along as the other is not itself a "filter" -
inputs/outputs EJBMetaData).

If there are multiple "filter"s then the getRelativeOrder() can be used to control ordering
(assuming it is important - but it is probably bad design?).

[alesj]

Yes. But you're welcome to examine the code and try to come up with new testcases
where it breaks. The only thing that should be not supported is loops. :-)
See DeployersImpl.sort()

Do we really need to check all current deployers (O(n^2))?
Since the original set is already ordered, thus we only need to find the position of the newly added deployer (O(n)).

[alesj]

Or O(n*log(n)) vs. O(log(n)) if we 'exaggerate' on the implementation. ;-)

[adrian.brock]

You'll be suprised how fast a "bubble sort" is with modern cpus that do caching. :-)

When we get to more than 100 deployers and it doesn't fit in the cpu cache
then we might want to look at optimizing it. :-)

It's actually much less than O(n^2) since (n-1) of the deployers are already in the correct
order anyway. :-)

[adrian.brock]

Since Carlo just found a bug in the sort, :-(
do you want to fix it?
http://www.jboss.com/index.html?module=bb&op=viewtopic&p=4059992#4059992

[alesj]

Since Carlo just found a bug in the sort, :-(
do you want to fix it?
http://www.jboss.com/index.html?module=bb&op=viewtopic&p=4059992#4059992

A chance to write that in O(log(n)). :-)
I already assigned a task to me.

[alesj]

I've re-written sorting.
I'm using simple transitive closure.
It sorts a lot better. :-)

The issue I'm having is with DeployerFlowUnitTestCase.testInputOutputMultipleTransient();

Should we make this work?
Since in my opinion this _is_ a loop.

[adrian.brock]

It's not a loop. The deployers are:

1) input=null, output=test
2) input=test, output=test
3) input=test, output=test
4) input=test, output=null

(2) and (3) are "filters", they input and output the same attachment.
If a deployer inputs and outputs the same attachment then it shouldn't
be considered as creating a loop.

The above is the correct order.
(2) and (3) looks ambigous, e.g. they could be other way around
but they are actually resolved using the Ordered.COMPARATOR.

In this case, the toString() on the deployer "3" is after "2" since the relativeOrders
are the same.

[adrian.brock]

What is this? It is not a fix to change the test.
You're supposed to make the tests pass, not change them!

The orders in those tests were the correct ones.

There's NO POINT having regression tests if you modify the tests
because you introduced a regression. :-)

> Modified: projects/microcontainer/trunk/deployers-impl/src/tests/org/jboss/test/deployers/deployer/test/DeployerFlowUnitTestCase.java
> ===================================================================
> --- projects/microcontainer/trunk/deployers-impl/src/tests/org/jboss/test/deployers/deployer/test/DeployerFlowUnitTestCase.java 2007-07-06 12:53:54 UTC (rev 63864)
> +++ projects/microcontainer/trunk/deployers-impl/src/tests/org/jboss/test/deployers/deployer/test/DeployerFlowUnitTestCase.java 2007-07-06 13:05:54 UTC (rev 63865)
> @@ -288,7 +288,7 @@
> assertEquals(6, deployer3.getUndeployOrder());
> assertEquals(5, deployer4.getUndeployOrder());
> }
> -
> +
> public void testMultipleOutput() throws Exception
> {
> DeployerClient main = createMainDeployer();
> @@ -453,11 +453,11 @@
> main.addDeployment(deployment);
> main.process();
>
> - assertEquals(1, deployer1.getDeployOrder());
> - assertEquals(2, deployer2.getDeployOrder());
> - assertEquals(3, deployer3.getDeployOrder());
> - assertEquals(4, deployer4.getDeployOrder());
> - assertEquals(5, deployer5.getDeployOrder());
> + assertEquals(5, deployer1.getDeployOrder());
> + assertEquals(4, deployer2.getDeployOrder());
> + assertEquals(1, deployer3.getDeployOrder());
> + assertEquals(2, deployer4.getDeployOrder());
> + assertEquals(3, deployer5.getDeployOrder());
> assertEquals(6, deployer6.getDeployOrder());
> assertEquals(-1, deployer1.getUndeployOrder());
> assertEquals(-1, deployer2.getUndeployOrder());
> @@ -469,33 +469,33 @@
> main.removeDeployment(deployment);
> main.process();
>
> - assertEquals(1, deployer1.getDeployOrder());
> - assertEquals(2, deployer2.getDeployOrder());
> - assertEquals(3, deployer3.getDeployOrder());
> - assertEquals(4, deployer4.getDeployOrder());
> - assertEquals(5, deployer5.getDeployOrder());
> + assertEquals(5, deployer1.getDeployOrder());
> + assertEquals(4, deployer2.getDeployOrder());
> + assertEquals(1, deployer3.getDeployOrder());
> + assertEquals(2, deployer4.getDeployOrder());
> + assertEquals(3, deployer5.getDeployOrder());
> assertEquals(6, deployer6.getDeployOrder());
> - assertEquals(12, deployer1.getUndeployOrder());
> - assertEquals(11, deployer2.getUndeployOrder());
> - assertEquals(10, deployer3.getUndeployOrder());
> - assertEquals(9, deployer4.getUndeployOrder());
> - assertEquals(8, deployer5.getUndeployOrder());
> + assertEquals(8, deployer1.getUndeployOrder());
> + assertEquals(9, deployer2.getUndeployOrder());
> + assertEquals(12, deployer3.getUndeployOrder());
> + assertEquals(11, deployer4.getUndeployOrder());
> + assertEquals(10, deployer5.getUndeployOrder());
> assertEquals(7, deployer6.getUndeployOrder());
>
> main.addDeployment(deployment);
> main.process();
>
> - assertEquals(13, deployer1.getDeployOrder());
> - assertEquals(14, deployer2.getDeployOrder());
> - assertEquals(15, deployer3.getDeployOrder());
> - assertEquals(16, deployer4.getDeployOrder());
> - assertEquals(17, deployer5.getDeployOrder());
> + assertEquals(17, deployer1.getDeployOrder());
> + assertEquals(16, deployer2.getDeployOrder());
> + assertEquals(13, deployer3.getDeployOrder());
> + assertEquals(14, deployer4.getDeployOrder());
> + assertEquals(15, deployer5.getDeployOrder());
> assertEquals(18, deployer6.getDeployOrder());
> - assertEquals(12, deployer1.getUndeployOrder());
> - assertEquals(11, deployer2.getUndeployOrder());
> - assertEquals(10, deployer3.getUndeployOrder());
> - assertEquals(9, deployer4.getUndeployOrder());
> - assertEquals(8, deployer5.getUndeployOrder());
> + assertEquals(8, deployer1.getUndeployOrder());
> + assertEquals(9, deployer2.getUndeployOrder());
> + assertEquals(12, deployer3.getUndeployOrder());
> + assertEquals(11, deployer4.getUndeployOrder());
> + assertEquals(10, deployer5.getUndeployOrder());
> assertEquals(7, deployer6.getUndeployOrder());

[adrian.brock]

If you're introducing new tests then do that, don't change the original tests. :-)

[alesj]

What is this? It is not a fix to change the test.
You're supposed to make the tests pass, not change them!

The orders in those tests were the correct ones.

There's NO POINT having regression tests if you modify the tests
because you introduced a regression. :-)

:-)

The new sort satisfies the given order.
In the cases where the position doesn't matter, it currently doesn't do name compare.
I'll see how I can push that in as well.

[alesj]

If you're introducing new tests then do that, don't change the original tests. :-)

It was easier to fix your tests than introduce name compare into the sorting algorithm. :-)

I'll revert the changes and add name compare on the one's that are not 'comparable'.