Created attachment 103952 [details]
the api change and implementation

the patch applies and builds.  The tests associated with j2eeserver also execute.

The factory classes associated with a 'server' usually include some data about the instance of the server when they construct an instance of their object.

That is not true for the ModuleConfigurationFactory.

As server's become more modular, there are situations where the ModuleConfiguration will depend on the features that are associated with a particular instance,

This allows plugin implementer's to create a ModuleConfiguration object that depends on a particular instance of a server.

First a general comment: j2eeserver has public API so you do not know who else might be implementing ModuleConfigurationFactory SPI outside of NetBeans source code repository and so incompatibly changing it the way you did is breaking the contract. If the API would be Friend and not Public then you could do this and fix all the clients of API as they must be listed in project.xml. Backward compatible way to deal with this situation is to introduce ModuleConfigurationFactory2 which extends ModuleConfigurationFactory and introduces new method. And ConfigSupportImpl from j2eeserver module would check whether moduleConfigurationFactory is instance of ModuleConfigurationFactory2 and use it in such case or fallback on good old ModuleConfigurationFactory. That way SPI clients can upgrade take advantage of new SPI as it suits them.

Looking at j2eeserver source code I would think that you do not need this API change. Correct me If I'm wrong please but each server instance has its own ModuleConfigurationFactory and therefore GF 3.1 can create different version from GF 3.0, no?

(In reply to comment #3)
> First a general comment: j2eeserver has public API so you do not know who else
> might be implementing ModuleConfigurationFactory SPI outside of NetBeans source
> code repository and so incompatibly changing it the way you did is breaking the
> contract.

Understood.

> If the API would be Friend and not Public then you could do this and
> fix all the clients of API as they must be listed in project.xml. Backward
> compatible way to deal with this situation is to introduce
> ModuleConfigurationFactory2 which extends ModuleConfigurationFactory and
> introduces new method. And ConfigSupportImpl from j2eeserver module would check
> whether moduleConfigurationFactory is instance of ModuleConfigurationFactory2
> and use it in such case or fallback on good old ModuleConfigurationFactory.
> That way SPI clients can upgrade take advantage of new SPI as it suits them.

Thanks.  That is a good suggestion.  I will try that strategy and submit a new api change request.

> 
> Looking at j2eeserver source code I would think that you do not need this API
> change. Correct me If I'm wrong please but each server instance has its own
> ModuleConfigurationFactory and therefore GF 3.1 can create different version
> from GF 3.0, no?

Yes. Each server instance has its own ModuleConfigurationFactory which is created based on the 'server type', since there is no access to the per-instance data in the constructor.  If I remember correctly... you were one of the folks requesting that we eliminate the separate gfv3ee6 and gfv3ee6wc server types.  If you are closing issue 191008, then this api change is unnecessary?

take back to alter and resubmit new change based on David's suggested change.

(In reply to comment #4)
> Yes. Each server instance has its own ModuleConfigurationFactory which is
> created based on the 'server type', since there is no access to the
> per-instance data in the constructor. If I remember correctly... you were one
> of the folks requesting that we eliminate the separate gfv3ee6 and gfv3ee6wc
> server types.  If you are closing issue 191008, then this api change is
> unnecessary?

No, I'm still all for merging these two server types. :-)

Extending factory with a new method is OK. I just thought that in this case there is one server type with two registrations in module layer and each registration can have different ModuleConfigurationFactory. But that might be shortsighted idea.

I'll provide the API change.

Created attachment 104020 [details]
updated patch

that looks good to me.

Does this issue need to get assigned back onto the apireviews queue?

Please review.

when do we think this api change will get committed?

Looks good to me as well. Go ahead Petr and commit it.

Formally we should wait for 7 days for review, but I understand you need this soon.

I pushed the change to web-main 549807a5f072. I'll keep this issue open for next 5 days in case somebody would have objections.

(In reply to comment #13)
> Formally we should wait for 7 days for review

I understand the need for 7 days window in case of a change in the core, but in the case of j2eeserver this is not necessary as all of the clients (or most of them) (that is You and Vince) agree with the change. :-) It is low impact change.

actually... I was just asking for an answer to the question, "when do we think this api change will get committed?".

I was not asking you to commit the change right now....

Integrated into 'main-golden', will be available in build *201012160001* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/549807a5f072
User: Petr Hejl <phejl@netbeans.org>
Log: #193255 Provide access to instance data for ModuleConfiguration

There were no objections and patch is already integrated. Closing as fixed.