Service provider module resolution <was> Re: Services/configuration/context webrevs

[Paul Sandoz]

On Aug 5, 2012, at 11:00 PM, Alan Bateman <Alan.Bateman at oracle.com> wrote:
> On 01/08/2012 01:59, Mandy Chung wrote:
>> 
>> Thanks for adding these tests.  It's a general issue.  As we discussed
>> it offline today, it'll be addressed in a separate changeset.  I know
>> Alan is going to review it too.  Once he reviews it, I can push this
>> changeset for you.
> Sorry for the delay, but I have finally got time to go this (webrev.04).
> 
> Overall I've come around to agreeing with approach. Clearly we could conjure up cases where the resolver would find a solution today but not find a solution with the changes. On the other hand it is very important that the solution (and selected versions) can be explained to the user and it isn't nice that the dependencies of a service module can change the outcome. So I think the approach makes things more predictable and should help the diagnostic output too.

> 

And i am gonna do the same for optional dependencies too, so this code is gonna change again. Thus the mandatory dependencies have to resolve and are not affected by any optional dependencies (service-based or otherwise).


> I went through the resolver changes in detail and don't see anything obvious wrong. Naming of variables is a bit more verbose than the existing code but that's no big deal. A minor point is that serviceInterfaceQueue is a LinkedList so the contains check will be O(n). No big deal now but worth adding a comment as such usages have a habit of biting us later.
> 

I was tempted to write a Deque implementation that was based on linked hash set :-) It's gonna change soon with support for optional dependencies, so there is no point adding such a comment.

Basically there is no need to cache optional stuff (services or otherwise) while traversing the dependency graph. Instead linear traversal of the partial configuration can be performed i.e. iteration of Resolver.modules, which needs to change to preserve order (and in general we need to preserve the topological order of the modules through out the resolving and linking process).


> I also went through the test changes and the replacement of the shell tests with tests based _Configurator/ModuleInfoBuilder and I think it looks good (took me a few minutes to properly tune into this but overall I think the test coverage has improved quite a bit). I guess I probably would drop the "runtime" directory and move runtime.sh up one level as services.sh.
> 

No, it needs to be separated out as a project, there is exploded source, and i want to open it in a IDE (plus it will be updated when moving to TestNG, notice that there is a test dimension for testing the various class loaders, a TestNG data provider can be used to abstract this). We also may have other projects in the future.

When this patch goes in i will be updating the runtime tests to include more test code related to service instance iteration failure.


> So overall I think is a good improvement and good work.
> 

Thanks.

Paul.


> (Mandy - as I was going through the tests I notice we've got the GPL with Classpath exception header on the tests. Also did we discuss -esa and -ea recently? Usually they are passed through via the jtreg -vmoption rather than needing to be in each shell test).
> 
> -Alan.