Deployers Ordering Issue Reopened

[ropalka]

Opening discussion to public

"alesj" wrote:

Hmmm ... I think the pattern is too simplistic.
Or too non-deterministic.

The problem is that once we fill in initial relations,
we check for transitions, but details like "ignore 6 as it's input/output for all" are too specific.

Previous use case we had was more deterministic, this one isn't.
Or at least I don't see it as such.

e.g.
We should then take the cycle, try to remove all dots that are in all deployers and in both input/output.
If all deployers fall into this "removal" + could be then re-ordered, then it's OK.

I don't think we're here to solve every possible ordering issue,
mostly just the most natural/simple one's.
Hence sometimes adding some order number / changing name is still a valid option.

[alesj]

Initial arguments on how to "order" cyclic configuration.

I consider it wrong because:

WSTypeDeployer sets output 7
WSDeploymentDeployer expects input 7 and sets output 8 (thus WSDeploymentDeployer follows WSTypeDeployer)
Finally WSDeploymentAspectDeployer expects input 8 (and thus it follows WSDeploymentDeployer and shouldn't be name compared with WSTypeDeployer because of 8 input expectation)
Just ignore 6 (in both inputs and outputs as they are not important
in this case, because they're specified on both input and output)

Richard

PS: Maybe I'm missing something?

[ropalka]

This thread is related to: JBDEPLOY-201

Some facts:
---
F1) Deployer is key JBoss AS abstraction
with essential ordering feature based
on stages, inputs and outputs.

F2) It is also possible to specify relative
order for particular deployer to
workaround "by name" deployers comparison
if it doesn't fit our needs.

F3) For deployers it is really important
to be executed in correct order.

Some complains:
---
Here's the list of problems of
current deployers ordering impl:

P1) Deployer inputs are not validated at all.

Usecase:

I can have PARSING deployer that produces
some meta data and REAL deployer consuming it.
If I'd forgot to "register" PARSING deployer and
would "register" REAL deployer, there'd be no
problem reported by AS about missing deployer
producing required input (to satisfy dependency).

Reality:

Deployer developers don't specify inputs (requirements)
just for fan. Inputs are there to specify dependencies
and to ensure correct deployers ordering.

P2) Domino sorting tries to compare uncomparable deployers.

Usecase:

Deployer1(outputs="1")
Deployer2(inputs="1",outputs="2")
Deployer3(inputs="2",outputs="3")

What should be the result of Comparator.compare(Deployer1,Deployer2)?

Reality:

Currently name comparison is used for such deployer comparison
(or relative order if specified).
How current domino storing algorighm works is it creates matrix
and fills in relative order for every deployer combination.
This is broken IMHO because two deployers are not comparable
in general (without knowing inputs/outputs of other deployers).
IOW deployer inputs/outputs don't represent context less grammar.

Some suggestions:
---
If I could be you
ad1) I would bring in shiny deployers code after rainy prototype
ad2) I would say good bye to the past and hello to the future
ad3) I would fix deployers ordering in AS trunk to ensure F1)

ad1) Doesn't mean your code is bad. It means you were probably out of
resources with bloody deadlines when writing it. IOW I understand the
reason of deployers code/architecture quality, but I think it's time
for review/rewrite (although it will affect BC) to ensure F1).

ad2) Means you shouldn't be aware to do things right.
Sometimes its really painful to fix some bugs, but when you postpone
bugs in critical abstrations, you're calling for serious problems
in the future.

ad3) You'd fix it in AS trunk ASAP because EAP5 is comming out soon
and you can be sure there will be many deployer developers that
will complain about this ordering issue and you'll need to fix it after all.

Conclusion:

Let's discuss this deployers ordering problem here and let's find the correct/proper solution for it. Forget about BC (especially if it's broken).
We're here to solve the problems and IMHO JBoss AS trunk is the best candidate for such fix to be incorporated in ;)

[ropalka]

"alesj" wrote:

I don't think we're here to solve every possible ordering issue,
mostly just the most natural/simple one's.
Hence sometimes adding some order number / changing name is still a valid option.

Yes, we're here to solve the problems especially for "key JBoss AS abstractions" like deployers ;)

[ropalka]

So the questions I'd like to discuss here are:

Q1) Is Domino Sorting algorithm a good choice?
Q2) Does it cover all sorting usecases?
Q3) Shouldn't we incorporate stages into account when comparing deployers?
Q4) Shoudn't we validate deployer inputs?

[ropalka]

Q1) Is Domino Sorting algorithm a good choice?

IMHO no

Q2) Does it cover all sorting usecases?

IMHO no

Q3) Shouldn't we incorporate stages into account when comparing deployers?

IMHO yes

Q4) Shoudn't we validate deployer inputs?

IMHO yes

What do you Ales think?

[alesj]

We've been down this road before, only to find out you had one valid argument.
And that was implemented as part of domino ordering.

Wrt P1), we've already discussed that, and my decision still stands.
Doing across stages checks is nothing more to me than supporting
someone else's laziness by not taking the time to do proper check or testing before.

As for P2) why would you even need to compare two deployers?
CompareTo code really comes in use when you as a user
actually get a hold of two instances and try to compare them.
Users/devs should never have to explicitly compare two deployers.

Wrt "suggestions". You came up with a clear case of cycle dependency, proposed some fuzzy logic solution,
put "bugs" label on existing code and you now expect us to drop everything to support your claims?

The reality is that apart from you we haven't had any deployers ordering issues.
I do give you credit for previous example and suggestion for a fix,
but in this case, not only you're repeating yourself, I think you're just plain wrong.

If something is a clear example of a cycle,
perhaps you can somehow "avoid" it per example use case,
but I doubt there is any nice non-complex aka human readable algorithm to solve it.

And that's what it matters at the end - users should be able to order deployers with simple paper drawing.
In my mind this definitely excludes your fuzzy/custom logic,
otherwise we would have to have a RichardO book of custum rules to follow. ;-)

[ropalka]

"alesj" wrote:
We've been down this road before, only to find out you had one valid argument.
And that was implemented as part of domino ordering.

Wrt P1), we've already discussed that, and my decision still stands.
Doing across stages checks is nothing more to me than supporting
someone else's laziness by not taking the time to do proper check or testing before.

We (i.e. JBossWS) don't need cross deployers stages to be taken into account.
I've been just wondering universal solution that would work for all the usecases.
IOW I've been thinking about your code trying to help you to make it generic and covering not just trivial usecases.

"alesj" wrote:

As for P2) why would you even need to compare two deployers?
CompareTo code really comes in use when you as a user
actually get a hold of two instances and try to compare them.
Users/devs should never have to explicitly compare two deployers.

Just FYI we don't compare deployers. It's deployers impl job.
The above compareTo was just an abstraction of your DominoOrdering.init() method (the abstration you're executing in every cycle). Again I was talking about your code ;)

"alesj" wrote:

Wrt "suggestions". You came up with a clear case of cycle dependency, proposed some fuzzy logic solution,
put "bugs" label on existing code and you now expect us to drop everything to support your claims?

Why is our usecase so special? We're expecting/providing e.g. JBossWebMetadata in both inputs/outputs because we're reading web MD and modifying MD. There's nothing wrong with it. It's exactly deployer inputs/outputs are intended for, don't they?

"alesj" wrote:

The reality is that apart from you we haven't had any deployers ordering issues.

And? Does it mean deployers ordering is correct? This is strange implication:(

"alesj" wrote:

I do give you credit for previous example and suggestion for a fix,
but in this case, not only you're repeating yourself, I think you're just plain wrong.

Thanks for the credits. We've been using the workaroud suggestion already for months.
We know about relative order workaround and we don't like it.
We'd rather to see it fixed in deployers impl code where it should be fixed.
Why we should do workarounds in our code if we're regularly using deployers and doing nothing special with them?

If you don't wanna fix this issue, just mark JBDEPLOY-201 as WON'T FIX and I won't waste my free time trying to help you.
For me deployers are "key abstraction" of AS and the fact is its input/output based ordering doesn't work for non trivial usecases.

PS: If you can live with it then you're lucky man.
My problem is I'm perfectionist and thus
I don't like partial solutions :(

[ropalka]

If you can live with it then you're lucky man.

Seems you're perfectionist too ;)
Thank you for the fix and collaboration Ales.
It's finally working with the latest fix applied to AS trunk.