Suffix ordering inconsistency

[starksm64]

In looking into the .har/.jar ordering change introduced with the har deployer removal: http://jira.jboss.com/jira/browse/JBAS-2232 it seems we are not really handling the new ehanced suffix notion consistently. Some of the existing problems I'm seeing are:

- There is too much reliance on the code level defaults and this is resulting in inconsistencies if a deployer is updated to override these settings. For example, the SARDeployer externalized its suffixes to ".sar,.sar/,.har,.har/,.deployer,.deployer/,service.xml,deployer.xml", but did not set its RelativeOrdering so it defaults to 500. A number of the suffixes in the xmbean descriptor are simply ignored because there is a pre-existing default with a different relative order coming from the SuffixOrderHelper. The end result is that a number of these suffixes are sorted incorrectly, but it largely did not matter because they are unused. Where it did matter is for the har deployer which previously was registering a 400:.har extended suffix, but is now 500:{.har,.har/}. I don't think there should be any code level defaults in SuffixOrderHelper. All code level defaults should be coming from the deployers themselves.

- There is no enhancedSuffix notion at the SubDeployerMBean level. There is only a Suffixes + RelativeOrder configuration which is used to build the enhanced suffix order as the cross product of Suffixes[] x RelativeOrder. There needs to be an ability to specify finer grain suffix ordering for a deployer at the SubDeployer base class implementation, and this is what should be queried by the MainDeployer.addDeployer(SubDeployer) method. This can be built from the Suffixes[] x RelativeOrder if there is no EnhancedSuffixOrder[] specified.

- There is an inconsistency in the handling of non-directory vs directory suffixes (.har vs .har/). One issue is the directory suffixes do not have defaults so there are invalid seperation of .sar vs .sar/ for example. That should be cleaned up by moving the default to the deployers themselves. The other is mentioned by Dimitris in JBAS-2000: suffixes that end with '/' are practically ignored by the DeploymentSorter, but it doesn't really matter if the suffix appears in both forms (i.e. .sar and .sar/). The effectively undoes first error since the 500:.sar/ suffix is treated the same as 200:.sar. Nothing like the two wrongs make a right school of coding.

So, the three things I think we need to do to clean this up are:

1. Remove all of the SuffixOrderHelper EnhancedSuffixOrder defaults.
2. Add support for EnhancedSuffixOrder to the SubDeployer/SubDeployerSupport. This would be an incompatible change as any SubDeployer not extending SubDeployerSupport would fail to deploy with a NoSuchMethodError. A SubDeployerExt interface that extends SubDeployer could be added with MainDeployer.addDeployer(SubDeployer) performing a runtime type check for the SubDeployerExt interface to determine how the EnhancedSuffixOrder should be obtained.
3. Fix the defaults of every jboss deployer and utilize the EnhancedSuffixOrder where deployers have externalized their suffix configuration.

[dimitris]

Scott, I was looking exaclty at that and solved most of it, except (2).

One issue is that there is indeed need for a single deployer to register suffixes with *different* suffix order, e.g. 050:.deployer,150:.sar, so we need more fine grained control there.

The solution I gave was to change the SubDeployer interface from:

 public String[] getSuffixes();
 public int getRelativeOrder();

to

 public String[] getSuffixes();
 public int[] getRelativeOrder();

so that each suffix can have a different relative order. (I know this breaks classes not extending SubDeployer support, OTOH, I got no complaint on JIRA, or forums or else when I added it, I guess everybody is using SubDeployerSupport :) I can move the new method to SubDeployerExt, if needed.

SubDeployerSupport has in addition a

 protected void setEnhancedSuffixes(String[] enhancedSuffixes)

that can do the parsing and set both arrays. If a subdeployer wants, it can externalize this setting, if not fine.

The I fixed the default SubDeployerSupport.accepts() so there is no need for SubDeployers to register both ".sar" and ".sar/" suffixes, as the .har deployer changes tried to do.

 * A default implementation that uses the suffixes registered
 * through either setSuffixes() or setEnhancedSuffixes(), to
 * decide if a module is deployable by this deployer.
 *
 * If (according to DeploymentInfo) the deployment refers to
 * a directory, but not an xml or script deployment, then
 * the deployment suffix will be checked also against the
 * registered suffixes + "/".

As a side note on this, from the VDF prototype it seemed, whether a deployment is packed or unpacked should be handled by the lower layers, and this saves us from a hundred places where we add a '/' or remove '/' or whatever, and greatly simplifies the code that deals with deployments.

Finally I tried to remove most hardcoded defaults from SuffixOrderHelper, and have the collective SuffixOrder built dynamically, but run into this:

When the DeploymentScanner makes its first pass on ./deploy, only the SAR and JAR deployer are registered, so initially the list of deployables is wrongly sorted!

Surely the deployers are deployed first (because the SAR deployer registers off-line .deployer) and the SuffixOrder is built correctly at the end, but the list is already sorted at the point where the Scanner calls the MainDeployer, so we get errors.

It seems, we need to first deploy the deployers to build the list, then everything else.

So, I've come to a point where half of the suffixes are registered dynamically, the other are static. I've run tests and everything and it seems ok, so I think we should go with this, update all deployers with their suffixes/relative order and everything, but leave the full dynamic registration for 4.0.4, because we need to decide how to deploy the deployers first.

I've started for a simple change and already spent 2.5 days on this, I'll check in my work later today...

[starksm64]

I don't like this change:

public String[] getSuffixes();
public int[] getRelativeOrder();

because I do know of external non-jboss subdeployer implementations, and its cumbersome to use. Generally a subdeployer does not differentiate between its deployment types. The SARDeployer is a special case because its essentially an extension point for all things being loaded into jboss.

If you want to be able to differentiate between suffixes for a given deployer you would need to list the extended suffix order:

"200:.sar,200:.sar/,400:.har,400:.har/,100:.deployer,100:.deployer/,200:service.xml,100:deployer.xml"

[starksm64]

The I fixed the default SubDeployerSupport.accepts() so there is no need for SubDeployers to register both ".sar" and ".sar/" suffixes, as the .har deployer changes tried to do.

Ok, that looks ok, but I want to look a bit more at the interaction between the deployers and deployment scanner. I agree that the knowledge of the packaging should ultimately only be a detail that the VFS is dealing with.

[starksm64]

Finally I tried to remove most hardcoded defaults from SuffixOrderHelper, and have the collective SuffixOrder built dynamically, but run into this:

When the DeploymentScanner makes its first pass on ./deploy, only the SAR and JAR deployer are registered, so initially the list of deployables is wrongly sorted!

Surely the deployers are deployed first (because the SAR deployer registers off-line .deployer) and the SuffixOrder is built correctly at the end, but the list is already sorted at the point where the Scanner calls the MainDeployer, so we get errors.

It seems, we need to first deploy the deployers to build the list, then everything else.

So, I've come to a point where half of the suffixes are registered dynamically, the other are static. I've run tests and everything and it seems ok, so I think we should go with this, update all deployers with their suffixes/relative order and everything, but leave the full dynamic registration for 4.0.4, because we need to decide how to deploy the deployers first.

Ok, but at a minimum the bootstrap extended suffix ordering should be specified via the org.jboss.deployment.MainDeployer-xmbean.xml descriptor so that it can be expanded/reduced as needed without code changes. Further, it should not be an exception to override a suffix deployment order as it currently is. Today we are just swallowing the RuntimeException thrown in this situation. Rather, the new suffix should override the existing one, and a debug level message logged.

[dimitris]

What do you mean by cumbersome? Interally the SubDeployer implementation can either do:

setSuffixes(new String[] { ".suf1", ".suf2" });
setRelativeOrder(300);
or just
setEnhancedSuffixes(new String[] "300:.suf1", "300:.suf2");

I wanted the enhancedSuffix ([order:]suffix) to be an implementation detail and not expose this it on the subdeployer external interface, if we decide we don't like this syntax.

[dimitris]

Ok, fine with the suffixes, I can remove all hardcoded settting and uncomment the entry in MainDeployer's -xmbean.xml.

[starksm64]

You can leave a minimal list, I just want this set from the MainDeployer descriptor and tested that this is what is really driving the defaults.

[starksm64]

What do you mean by cumbersome? Interally the SubDeployer implementation can either do:

Its cumbersome to match two long sets of configurations from the descriptor:

 <attribute access='read-write' getMethod='getSuffixes' setMethod='setSuffixes'>
 <description>Get an array of suffixes of interest to this subdeployer</description>
 <name>Suffixes</name>
 <type>[Ljava.lang.String;</type>
 <descriptors>
 <value value=".sar,.sar/,.har,.har/,.deployer,.deployer/,service.xml,deployer.xml" />
 </descriptors>
 </attribute>
 <attribute access='read-write' getMethod='getRelativeOrder' setMethod='setRelativeOrder'>
 <description>Get the relative order of the specified suffixes</description>
 <name>RelativeOrder</name>
 <type>int[]</type>
 <descriptors>
 <value value="200,200,400,400,100,100,200,100" />
 </descriptors>
 </attribute>

Does this match the previous extended suffix list I sent out? Changing one attribute at a time also leads to a non-atomic update of what is really a composite attribute. In short, I don't think there is a writable RelativeOrder attribute of type int[]. There is a RelativeOrder order scalar convience setting that applies the same ordering to all suffixes, and there is the inherent ExtendedSuffixOrder list which is the natural attribute since this is actually what is used internally. I do think the extended suffix syntax should be exposed as the fine grained control mechanism.

[dimitris]

I've made/tested the changes we discussed.

- added a SubDeployerExt/SubDeployerExtMBean that has the additional EnhancedSuffixes attribute, to be used by the MainDeployer when implemented to retrieve suffix/order.

- all deployers define and externalize their default EnhancedSuffixes (except for the ClientDeployer)

- there is no hard-coded config in SuffxOrderHelper.

- MainDeployer-xmbean.xml defines the following "static" sequence:
250:.rar,300:-ds.xml,400:.jar,500:.war,600:.wsr,650:.ear,800:.bsh,900:.last

- the rest is build dynamically as deployer are registered:
050:.deployer,050:-deployer.xml,100:.aop,100:-aop.xml,150:.sar,150:-service.xml,200:.beans,250:.rar,300:-ds.xml,350:.har,400:.jar,450:.ejb3,450:.par,500:.war,600:.wsr,650:.ear,700:.jar,750:.zip,800:.bsh,900:.last

- I guessed a relative order for the new .bean deployer, just after the .sar deployer

- in most cases SubDeployer.accepts() can do the job, based on the configured EnhancedSuffix

I've left the MainDeployer defining "static" suffixes that are not changed by the SubDeployers. Changing the logic to the complete opposite is something I would like to give some more thought.

[starksm64]

Looks pretty good so far and the har/jar ordering is working in my testing. I'll more into the static suffix issue. Are you saying that there is a problem with not having a static set of suffixes?

[dimitris]

Trying to remove the static ones, gave me a problem in the 'all' configuration, when deploying the content under the ./deploy/management directory.

I tracked this down to not having the '.war' suffix in place, in time.

Since I don't know what other consequences it may have, I've put this static order that together with the early registering JAR/SAR deployers should give exactly the same suffix order we've been used so far, to avoid any problem with 4.0.3.

Maybe what we can do next, is to have an initial run of the DeploymentScanner using an once-off filter to weed out everything except for the ./deployer deployments, then proceed normally to deploy everything else.