This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 141427 - formalize api for safely adding a default instance
Summary: formalize api for safely adding a default instance
Status: RESOLVED FIXED
Alias: None
Product: serverplugins
Classification: Unclassified
Component: Infrastructure (show other bugs)
Version: 6.x
Hardware: PC All
: P3 blocker (vote)
Assignee: apireviews
URL:
Keywords: API, API_REVIEW_FAST
Depends on:
Blocks: 138554
  Show dependency tree
 
Reported: 2008-07-23 22:07 UTC by Vince Kraemer
Modified: 2008-07-31 15:43 UTC (History)
1 user (show)

See Also:
Issue Type: ENHANCEMENT
Exception Reporter:


Attachments
diffs to j2eeserver. includes usage of the API (4.51 KB, text/plain)
2008-07-23 23:37 UTC, Vince Kraemer
Details
a second round of diffs to address some of phejl's comments (10.22 KB, text/plain)
2008-07-25 01:23 UTC, Vince Kraemer
Details
serverplugin initialization patch (16.96 KB, text/plain)
2008-07-25 19:24 UTC, Petr Hejl
Details
forgot to add one test file - fixed (19.82 KB, text/plain)
2008-07-25 19:59 UTC, Petr Hejl
Details
updated patch (20.13 KB, text/plain)
2008-07-27 09:43 UTC, Petr Hejl
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Vince Kraemer 2008-07-23 22:07:04 UTC
many of the plugins need/want to register a default instance.  Most all of them have had to write code at the FileObject
level into the server registry, since the registration of a default instance needs to take place before the registry is
fully initialized.

This change would create a clear place to code the default instance registration... that will be triggered AFTER the
server registry is initialized.
Comment 1 Vince Kraemer 2008-07-23 23:09:00 UTC
Note this change would also allow plugin developers to stay inside the registry apis, instead of being forced to code
below the registry abstraction at the FileObject level.
Comment 2 Vince Kraemer 2008-07-23 23:17:57 UTC
issue 138554 could be resolved by implementing a hack similar to those used in the Tomcat, JBoss and GlassFish
plugins... so this isn't a strict "blocker"... but it would be better to implement this API and avoid implementing more
of the same code that breaks the ServerRegistry abstraction.  Once the API is in place, the hacks in the existing code
could be re-evaluated by the module owners.
Comment 3 Vince Kraemer 2008-07-23 23:37:17 UTC
Created attachment 65441 [details]
diffs to j2eeserver. includes usage of the API
Comment 4 Vince Kraemer 2008-07-23 23:48:13 UTC
Broken encapsulation examples

http://hg.netbeans.org/main/file/tip/tomcat5/src/org/netbeans/modules/tomcat5/TomcatFactory.java : method
registerServerInstanceFO

http://hg.netbeans.org/main/file/tip/j2ee.jboss4/src/org/netbeans/modules/j2ee/jboss4/JBDeploymentFactory.java : method
registerDefaultServerInstance
Comment 5 Petr Hejl 2008-07-24 16:29:10 UTC
PH01: I have no idea how this API change would be used in your current code. I tried to design an alternative approach
without callback (register method callable anytime), but it was impossible for me to incorporate it to the GFv3
workflow. It is even not clear how SPI change designed in this isuue would be used. Please post the patch that will
include the real usage - such as GFv3 registration.

Maybe I'm missing something but I'd really like to know how this will work with GFv3.

PH02: The unit test is needed for any change to ServerRegistry (not actually needed right now, until PH01 is solved).
PH03: Wrong module version.
Comment 6 Vince Kraemer 2008-07-24 16:53:19 UTC
http://www.netbeans.org/issues/show_bug.cgi?id=138554#desc7 contains additional information that supports the need for
an API.
Comment 7 Petr Hejl 2008-07-24 23:16:16 UTC
I'm not arguing about a need for API. It is just not clear to me how this SPI will solve the problem (I'm not saying it
won't solve it). Any patch introducing the API/SPI should contain its usage. Nothing more.

Using the API is also the way how to test the design of the API.
Comment 8 Vince Kraemer 2008-07-25 01:23:34 UTC
Created attachment 65607 [details]
a second round of diffs to address some of phejl's comments
Comment 9 Vince Kraemer 2008-07-25 06:27:48 UTC
Most of the comments that follow are based on the context of the diffs in forReview2.txt...

RE PH01:

The additional spi that I am proposing is in OptionalDeploymentManagerFactory.java

The additional method is boolean registerDefaultInstance().  This method returns true, if an instance is registered and
false if an instance does not need to be registered.  It throws an InstanceCreationException if there is some failure
while attempting to register the instance.  This is different from my original definition of the SPI.  I figured folks
would like to be able to detect problems on the "client side" of this SPI and "react" accordingly.

An example of the usage of the spi is in ServerRegistry.java.  This usage is clearly visible in the diff.

An example implementation of the spi is in Hk2OptionalFactory.java. This implementation is clearly visible in the diff.

RE PH02:

I have extended the ServerRegistryTest.java to exercise the additional SPI.

RE PH03:

I have updated the module version... though I will obviously need to double check this BEFORE the API change is pushed
to main. 
Comment 10 Petr Hejl 2008-07-25 19:23:13 UTC
PH01: I believe we should make the introduced SPI method more generic. It is also called after known instances are
loaded - initialization code can manipulate them (check existence, delete etc.). Maybe it would be beneficial to let the
method throw a checked exception. I didn't find usecase for return value.

PH02: Unfortunately introduced test did not really test the registry but the mock test plugin. Fixed.

Attaching the proposed patch.
Comment 11 Petr Hejl 2008-07-25 19:24:05 UTC
Created attachment 65665 [details]
serverplugin initialization patch
Comment 12 Petr Hejl 2008-07-25 19:59:16 UTC
Created attachment 65669 [details]
forgot to add one test file - fixed
Comment 13 Vince Kraemer 2008-07-25 20:06:15 UTC
in response to points raised in http://www.netbeans.org/issues/show_bug.cgi?id=141427#desc11

PH01: more generic -- great idea.
      push to after instance initialization -- seems ok... may not address issue 138554. I will double check.
      checked exception -- I could go either way... your version of the SPI doesn't use a checked exception. since the
code that is going to catch the exception is in j2eeserver, I will defer to you....

PH02: yup. the test was weak... yours is better.

Thanks for such a thorough review.
Comment 14 Vince Kraemer 2008-07-26 19:13:45 UTC
VK01: I did think of one thing that I would like to change... the name of the method added in ODMF...

The name initializePlugin would lead people to think that the method is called very early in the lifecycle of the
plugin.  And it isn't. It is actually called pretty late in the lifecycle of the plugin. It may be as cearer to name the
method finishInitialization()...

Comment 15 Petr Hejl 2008-07-27 09:40:19 UTC
Re VK01: Method renamed to finishServerInitialization. Added checked exception to provide a clue to an implementor what
to do under unexpected conditions.

I suppose this could be the final patch.
Comment 16 Petr Hejl 2008-07-27 09:43:34 UTC
Created attachment 65743 [details]
updated patch
Comment 17 Vince Kraemer 2008-07-27 17:02:57 UTC
Yes. I think this can be the final patch....

Will you be doing the commit/push or do you want me to do it?

If you do it... please do not check in the code changes that are in the glassfish.common module.  I will commit those,
since they are actually for a bug fix and not the API change. 
Comment 18 Petr Hejl 2008-07-27 17:29:59 UTC
As I'm on vacation next week, it would be great if you could commit/push this.
Thanks.
Comment 19 Vince Kraemer 2008-07-27 20:56:23 UTC
will do. have a nice vacation.
Comment 20 Vince Kraemer 2008-07-31 05:17:55 UTC
http://hg.netbeans.org/main/rev/01260e21347f
Comment 21 Quality Engineering 2008-07-31 15:43:46 UTC
Integrated into 'main-golden', available in build *200807311401* on http://bits.netbeans.org/dev/nightly/
Changeset: http://hg.netbeans.org/main/rev/01260e21347f
User: vkraemer@netbeans.org
Log: #141427 : api change to allow plugins to finish initialization.