Modified calculating order for DefaultStateTransitionComparator

[robertmcnees]

Expanded the functionality of the DefaultStateTransitionComparator to match the documentation and catch edge cases.

The original documentation still holds:

* > foo* > ??? > fo? > foo

And I more complex scenarios are now also covered:

* > foo* > *f* > *foo* > ??? > ?o? > foo?? > bar?? > fo? > foo > bar

[robertmcnees]

Fix for #3996

[fmbenhassine]

It is true that this example sorts the patterns from most generic to most specific (ie in ascending order of specificity), but the comparator itself returns the most specific pattern first and the least specific pattern last. This means the specificity is decreasing, not increasing. I know this is subtle, but I think the initial javadoc Sorts by decreasing specificity of pattern is correct (which I seem to have missed when I reviewed #4504). Do you agree?

[robertmcnees]

As you said this is subtle so I'm OK with leaving it as is. I've been confused by this many times and I think the more important part is the example in the documentation than the word ascending/descending.

However to detail my thinking for anyone who stumbles on this later:

the comparator itself returns the most specific pattern first

The way I read it the comparator considers the more generic patterns to be greater than the more specific. So the comparator would return * > ?foo > foo, which I would call ascending order of specificity (most specific last).

This was the confusing part for me (quoting myself from a separate issue):

the order of specificity switches between the DefaultStateTransitionComparator and what is used in the FlowJobBuilder. That is while DefaultStateTransitionComparator will return * > foo* > ??? > fo? > foo, the FlowJobBuilder will use the most specific case first. This turns the example backwards to foo > fo? > ??? > foo* > *.

In short I think the DefaultStateTransitionComparator sorts in ascending order of specificity, but FlowJobBuilder sorts in descending order of specificity. FlowJobBuilder and descending order of specificity is what the end user interacts with, so maybe it's best to keep the documentation as 'descending' throughout.

[fmbenhassine]

Thank you for the clarification, @robertmcnees ! I see, my interpretation of the natural ordering of elements (ie the meaning/definition of the > symbol) in the DefaultStateTransitionComparator was inaccurate.

In short I think the DefaultStateTransitionComparator sorts in ascending order of specificity, but FlowJobBuilder sorts in descending order of specificity.

I believe this is the root cause of the confusion. There is probably a good reason for that (probably the use of TreeSet in SimpleFlow?), but my thinking is that since we want transitions to be sorted by the most specific first (like in the issue's example foo* should be fired before *), then the comparator should result in foo* being lower than *. Similar to the natural ordering of numbers, the analogy I like to compare to is that if I assign weights to transitions (like foo* -> 1 and * -> 2), the "natural" ordering coming out of the comparator itself would rank foo* lower than * (that's the "natural" order I was expecting, not the opposite).

It would be great if we try to avoid the ordering to be opposite between the comparator and the flow implementation. This is important because as a Batch user, if I want to provide a custom comparator, I would not have to do this mental effort to think backward in the comparator's implementation. Otherwise, we can keep the changes in this PR and add a clear definition of what ">" means in this context.

[robertmcnees]

That's a great point about someone wanting to create their own comparator. I was only thinking about it from my narrow perspective of debugging the code. I think making the DefaultStateTransitionComparator match FlowJobBuilder makes even more sense with that consideration.

I created a new issue #4527 so that we can track the solution there. It will likely involve changes that are out of scope to this PR and issue #3996.

[fmbenhassine]

I like this! A simple naming change makes the tests immediately more readable.

[fmbenhassine]

I tested this PR and the reported edge case where the pattern * is returned in priority instead of foo* (due to alphabetical sorting when both patterns have the same number of *) is now fixed without any regression 👍

@robertmcnees I just wanted to check with you the subtle detail about the decreasing / increasing wording in the javadoc before merging the PR. Other than that, LGTM.

[robertmcnees]

Thanks for the review, @fmbenhassine! I left a comment on the decreasing / increasing conversation, but I think it's very subtle and I'm fine with either way.

[robertmcnees]

I created a new issue #4527 to better track how FlowJobBuilder interprets the results from DefaultStateTransitionComparator. That issue could be handled independently of this PR and issue #3996. However given that this is a slight breaking change, I see the issue targeted for the 5.2.0 release which means we have time.

What is your preference @fmbenhassine? Would you rather handle the new issue #4527 in a different PR or would you rather adapt this one to have a single PR that covers both issues (#4527 and #3996)?

[fmbenhassine]

Yes, I would rather handle #4527 in a different PR as it might involve changes that are out of scope to this PR and issue #3996.

[fmbenhassine]

Rebased and merged as 9d139b5. Thank you for your contribution!