PH01: Looks like you already have some prototype. Can you provide a patch? Wiki with links to java source is very hard to read.
PH02: I don't really understand the StartServer.supportsProfilerStop() - "It is used for J2EE servers which wants to perform some additional clean up on stop". Either the method has wrong name or the description of its purpose is confusing.
PH03: In my opnion "Statefull" listeners may cause a lot of troubles later (due to the state and its synchronization). Maybe it would be better to have just ChangeListener and ChangeEvent. Client would query the state on its own.

Created attachment 96895 [details]
patch

>PH01
See attached patch.
>PH02
Stop of J2EE server is performed via StartServer.stopDeploymentManager(). This 
method is called also when there is need to stop J2EE server which was started
in profile mode. But there could be also a situation when JVM of J2EE server 
was started and blocked by Profiling agent. It means that J2EE server is not really started ( main method of main class is not called ). In the latter case 
generally method StartServer.stopDeploymentManager() should not be called. 
Implementation of this method usually connects to J2EE server admin port for stopping it. But server was not really started so if method tries to connect
to J2EE server port it will fail. Sometimes however J2EE StartServer 
implementation can require to do some clean up after stopping JVM blocked by 
Profiling agent . Such server should return true on supportsProfilerStop() method 
call. In this case StartServer.stopDeploymentManager() will be called even if JVM 
was blocked by profiling agent. 
>PH03
I think you are right. It seems it is more safely.

PH01:
Thanks.

PH02:
Ok. Could you please update a wiki? I still have doubts about the method supportsProfilerStop(). It looks like nobody will override it (at least now). Am I right? Perhaps it should not be present.

On the other hand if it would be overridden the stopDeploymentManager() is called in situation when there is no profiler info as it is already finished and cleared. In such situation maybe some method "public void profilerTerminated()" with empty body intended to be overridden by cleanup action (if needed) would be better choice. Such method would be called unconditionally and there would be no is/do pair... Anyway, do we need it now?

PH04:
For ProfilerSession.notifyStop() I would consider a different name. Is it really notify when it returns ProgressObject? Perhaps something like performStop would be a better choice.

Non API notes:

PH05:
ServerRegistry:
+                    throw new IllegalArgumentException(NbBundle.getMessage(
+                            ServerRegistry.class, "ERR_MultipleProviders" ,
+                            id, found.getClass(), provider.getClass()));

I think the proper exception to throw is IllegalStateException as argument passed is quite ok (the state is wrong).

PH06:
Field naming pattern myXXX is bit strange. Especially field myLock is saying nothing about its purpose.

>PH02
I can't describe the purpose of this method more accurately than I did.
Could you please look at proposed implementation of stateChanged() method in 
ServerInstance class ?
This implementation is exactly about my description.

The answer to the question about need of this method : YES. 
JBoss requires to call its method stopDeploymentManager() even if JVM blocked by 
agent. I don't exactly why but this is out of my scope .

There is an option to avoid this method with other method . 
F.e. stopBlockedProfiling() which will be called instead of 
stopDeploymentManager() method when JVM of J2EE server is blocked by profiler 
agent and J2EE server was not started.
J2EE server plugin will override the empty implementation.

>PH04
notifyStop() method probably is not the best name here . I agree. 
I don't like also "stop()" or "shutdown()" as it was previously . 
In the most of cases this method just notify listeners of ProfilerSession about
stopping event.
There is only one case when this method really perform "stop" action.
It is exactly the same case which is discussed in previous item:
only profiler implementation have control on JVM when it is blocked by profiler 
client ( again J2EE server is not actually started yet ) . This is only case 
when profiler stops target JVM.
This is why I don't like unconditional names for this method.
May be "requestStop" is better here.

(In reply to comment #5)
> >PH02
> I can't describe the purpose of this method more accurately than I did.
What I meant was to copy the description you provide here to wiki (for others interested in it). I understand the purpose now. Thanks.

> The answer to the question about need of this method : YES. 
In such case overriden method in JBoss should be part of the patch.

> JBoss requires to call its method stopDeploymentManager() even if JVM blocked
> by 
> agent. I don't exactly why but this is out of my scope .
I'll look into it.

> There is an option to avoid this method with other method . 
> F.e. stopBlockedProfiling() which will be called instead of 
> stopDeploymentManager() method when JVM of J2EE server is blocked by profiler 
> agent and J2EE server was not started.
> J2EE server plugin will override the empty implementation.
That's exactly my suggestion.

> >PH04
> May be "requestStop" is better here.
Yep. Looks better.

Thanks.

Comments unrelated to the API review:
 - is it really correct to have Target Milestone 6.9?
 - I don't like the provided patch, it mixes code changes with formatting changes, this makes it harder to read and understand

JS01: The name ProfilerProvider suggests that the class provides a profiler, I'd expect it to have a 'public Profiler getProfiler()' method. But it directly provides the logic, so I'd suggest to keep the 'Profiler' name or use something like 'ServerProfiler' if you don't like just 'Profiler'.

JS02: ServerRegistry.getProfilerProvider(Object id) uses ProfilerProvider.isApplicable(id) to find the appropriate profiler. I haven't found any code where this ID is generated/used. There's also no description in the method JavaDoc how the ProfilerProvider should handle & process the ID. Wouldn't it be much simpler to replace ProfilerProvider.isApplicable(Object id) by 'String ProfilerProvider.getId()' to be able to reference & store it?

JS03: ServerRegistry.getProfilerProvider(Object id) performs a check for more ProfilerProviders accepting the same id. What this situation actually means? If it means an error state, shouldn't we check it immediately when registering the ProfilerProvider?

JS04: I think that ProfilerProvider.getDefaultServerSettings() should stay ProfilerProvider.getServerSettings(). There's nothing like default settings, each profiler uses its own original settings and each session requires to have an unique agentId - at least for the NetBeans profiler - so each settings instance will be unique. Based on that, what about these changes in ProfilerProvider:
 - changing 'ProfilerSession getSession(ProfilerServerSettings)' to 'ProfilerSession createSession()'
 - changing 'ProfilerServerSettings getDefaultServerSettings(String serverInstanceID, boolean verbose)' to 'ProfilerServerSettings getServerSettings(String serverInstanceID, ProfilerSession session, boolean verbose)'
The code flow in TargetServer and ProfileAction would be then changed to:
 ProfilerSession session = provider.createSession();
 ProfilerServerSettings settings = provider.getServerSettings(instance.getUrl(), session, false);
This would allow the profiler to create a ProfilerSession with an unique ID if needed and this ID could be also used as an agentId for ProfilerServerSettings. It would also give the profiler an opportunity to immediately reject new profiling session (for whatever reason like it's already profiling another application) by returning null from createSession().

JS05: Related to JS04, since the ProfilerServerSettings are unique for each server & session I don't see any reason to introduce ProfilerServerSettings.setJvmArgs and ProfilerServerSettings.setEnv. Once the profiler generates the settings it doesn't need to be further modified. These methods don't seem to be used in the patch anyway.

JS06: I don't understand the introduction of ProfilerServerSettings.getProperties(). It doesn't seem to be used anywhere. Moreover, no project-related settings are needed/used for starting a server in profiling mode, providing the JDK & arguments (& optional env) is enough. It looks like these properties should be stored in ProfilerSession - also related to JS04.

>JS01
Profiler or ServerProfiler name means that implementors do profile.
But this is not true. They don't perform profiling.
They provides capabilities to manage J2EE server profiling.
ProfilerProvider ( current name of interface ) even doesn't manage 
profiling . This class should be implemented by third party providers of 
profiling capability. Profiling is managed by ProfilingSession.
So names supposed by you (and original name ) is completely confusing.
It can be ProfilingCapabilityProvider but current name is better from my point 
of view.
>JS02
getId() method is obvious here.
But I don't see the simple way to avoid collisions between different providers.
Now we have  NB Profiler. 
Please give me what id should be used for NB profiler ?
Also please give me the algorithm which will prevent identifier collisions
in different providers.
isApplicable(Object) method could return true not just for one identifier.
F.e. NB profiler could be applicable to implementation class ( .class object ) 
which is unique. It also could be applied to some other ( String ) id.
Actually getId() and isApplicable() do the same things . But isApplicable() 
could be widely used.
>I haven't found any code where this ID is generated.
There is no such code. Generation of such ID is out of scope this API.
But there is a usage of isApplicable() . Please look at ServerRegistry.getProfilerProvider( ). Please also look at comments in this 
method how id could be accessed from NBPreferences.
From very beginning I said to you that J2EE Profiler API is related to common Profiler API which doesn't exist. Such API could provide functionality to get/work with identifiers. But it doesn't exist. There is a need to distinguish various profiler providers. I propose the way to do this. May be not a best way.
But getId() method doesn't have any preference against isApplicable().
Also StartProfiledServer Ant task has an optional attribute which could be
used for choose special provider. It could be set per project.
>JS03
Yes , it prevent collisions.
> What this situation actually means? If it means an error state, shouldn't we check it immediately when registering the ProfilerProvider?
WHAT registration do you mean? I don't now explicit registration of service 
provider. I was always thinking that service provider is just declared as 
provider for some interface ( via annotation or META-INF) . So there is no any
special action for registration where possible to check collisions.
So the only way to check collision is to do this on reading available classes.
This is exactly done. If you have other way to REGISTER and error check before
reading please let me know.
>JS04
Method getServerSettings() is absolutely confusing. 
Probably the name gertDefaultServerSettings() is not the best. I agree. 
But it should somehow tell that this is 'special' ProfilingServerSettings.
Let's imagine we have getServerSettings() method. From it's name I suppose to 
call it for getting settings for session in any case.  But this is bad when
one wants to start server based on settings which was received from Project.
getServerSettings() should be used ONLY for getting settings for J2EE server start without any additional information available ( which is present when 
server is started for Project ). In other case ProfilerServerSettings should 
be instantiated directly. See StartProfiledServer Ant task class.
So getServerSettings() name is very bad.
Your further code flow in this item is ONLY for starting J2EE server in profile mode. In your suggestion there is no need in ProfilingServerSettings at all. 
Provider can create ProfilerSession in this case and no need to get ProfilingServerSettings .
But ProfilerSession should be created based on settings which are configured 
based on Project info ( when profiler started for Project ).
Such method : provider.createSession() should not return null. Because ServerInstance class is responsible for rejecting/starting server. This is the 
only correct place where one can stop other sessions of Provider if they exist. 
So even if session cannot be started when it requested it doesn't mean that
it can't be started later. ServerInstance could stop other sessions and let
current session be configured for starting. Your scenario doesn't allow this.
Please look at code in ServerInstance _startProfiling() method.
Also please note that possibility to start simultaneous sessions depend on 
provider . One could allow this but other could not.
>JS05
There is a mention on Wiki how NB profiler could be used for starting multiple
sessions. Let's assume NB Profiler implementation is changed and its not 
singleton. In this case network port number should differ for various 
sessions. J2EE Profiler implementation is a good place for choosing appropriate 
port number . But port number now is hardcoded in JVM arguments. 
Mentioned methods ( setJvmArgs ) could be used for modifying Java arguments 
with different port number.
>JS06
Ant properties are used for attaching prtofiler . See implementation of 
method attachProfiler(). Currently Agent Id ( also port number ) magically
appears in J2EEProfilerImpl . Properties could be used ( and in my 
implementation they are perfectly works)  for transfer some information 
between start profiling client and J2EE Profiler implementation. At least 
agent id should be placed there . Such agent id/port number receiving is 
more preferable than current unclear magical appearing.

(In reply to comment #10)
> >JS01
> Profiler or ServerProfiler name means that implementors do profile.
> But this is not true. They don't perform profiling.
Using a naming convention without following the pattern is not a good API design. I agree that 'Profiler' or 'ProfilerServer' is not the best solution, what about 'ProfilingSessionProvider' or 'ProfilerContext'? Feel free to use any other name, it just shouldn't confuse the API users - '{Subj}Provider' classes are expected to provide {Subj}.

> >JS02
> getId() method is obvious here.
> But I don't see the simple way to avoid collisions between different providers.
> Now we have  NB Profiler. 
> Please give me what id should be used for NB profiler ?
First of all the current state is wrong since it doesn't allow to implement the isApplicable() correctly. If I wanted to implement a new ProfilerProvider the provided JavaDoc doesn't tell me what the ID is and how I should decide whether it should be accepted. On the other side getId() is a common practice whenever you need to handle an unique object. It's up to the implementing class to decide which ID will be used (classname or whatever). Handling collisions could be left as it currently is for isApplicable(). Alternatively you can ignore collisions by picking up the first provider which accepts/provides the ID.

> >JS03
> Yes , it prevent collisions.
I see, thanks for the explanation.

> >JS04
> Method getServerSettings() is absolutely confusing. 
> Probably the name gertDefaultServerSettings() is not the best. I agree. 
> But it should somehow tell that this is 'special' ProfilingServerSettings.
> Let's imagine we have getServerSettings() method. From it's name I suppose to 
> call it for getting settings for session in any case.  But this is bad when
> one wants to start server based on settings which was received from Project.
ProfilerServerSettings (actually should be ProfiledServerSettings) is a wrapper class for the (and only the) settings required for starting the server for profiling. Neither the Java platform to use nor the JVM arguments are defined in the Project, it's generated by a concrete profiler for the concrete session (either using the ant task or from ProfilerProvider.getDefaultServerSettings). There's nothing like 'default' or 'special' settings, the settings are always required every time a server is being started for profiling and each settings instance is unique. Since the API implementors only create the settings for starting from Services tab you don't need to differentiate this from instantiating in StartProfiledServer which is part of private implementation. Feel free to use different method name other than 'getServerSettings()' but the name shouldn't suggest that there are any 'default' settings for starting a server.

> >JS05
> There is a mention on Wiki how NB profiler could be used for starting multiple
> sessions. Let's assume NB Profiler implementation is changed and its not 
> singleton. In this case network port number should differ for various 
> sessions. J2EE Profiler implementation is a good place for choosing appropriate 
> port number . But port number now is hardcoded in JVM arguments. 
> Mentioned methods ( setJvmArgs ) could be used for modifying Java arguments 
> with different port number.
I can't find where the port number is hardcoded, even in the current implementation the port number can be different for each session. If not it's a bug in NB profiler which should be unrelated to this API. Let's forget about the NB profiler for a while - the profiler should setup the server settings with an appropriate port number when instantiating the settings, there's no reason to modify it later.

> >JS06
> Ant properties are used for attaching prtofiler . See implementation of 
> method attachProfiler(). Currently Agent Id ( also port number ) magically
> appears in J2EEProfilerImpl . Properties could be used ( and in my 
> implementation they are perfectly works)  for transfer some information 
> between start profiling client and J2EE Profiler implementation. At least 
> agent id should be placed there . Such agent id/port number receiving is 
> more preferable than current unclear magical appearing.
Here we're polluting the ProfilerServerSettings with an unrelated information. This class contains information required for starting the server JVM for profiling (see class' JavaDoc), it shouldn't be used for sharing data between the J2EE framework and profiler implementation. I thought that ProfilerSession would be more appropriate if the project-related settings are needed at all, but I'm fine with any other approach...
Regarding the id/agent port - these are profiler-private implementation details unrelated to the API (each profiler requires a different set of settings), the NB profiler stores this in ProfilerServerSettings.jvmArgs which is general enough from the serverplugin viewpoint.

I'm afraid the discussion is not very constructive, each of us have a different mental model of how the API actually works. I believe this is the reason for most of the misunderstandings.

From my viewpoint the API itself is not a feature, the API is just a way to enable implementing features. Without a reference implementation and/or a detailed description of how the API is meant to be implemented/used I'm not able to decide whether the features are really enabled by the API.

I'm suggesting to update the current profiler.j2ee implementation to use the new API - this will precisely illustrate the API usage and prove that the API fulfills the designed goals. Does anyone on cc have any other idea how to proceed further?

(In reply to comment #12)
> I'm afraid the discussion is not very constructive, each of us have a different
> mental model of how the API actually works. I believe this is the reason for
> most of the misunderstandings.

I agree. AFAIK there is one good rule for API review (and for any code in fact) - to not to push anything not used at all to our codebase.

Either API is useless (not used by anybody) and should not be pushed or it should be pushed together with the code which is the API trying to fix/improve/enhance.

It is also a good learn-by-example approach for other potential API users.

>JS01
I disagree.
Here is examples which is already in NB API :
http://bits.netbeans.org/dev/javadoc/org-netbeans-modules-refactoring-api/org/netbeans/modules/refactoring/spi/ProgressProvider.html
http://bits.netbeans.org/dev/javadoc/org-netbeans-modules-projectapi/org/netbeans/spi/project/ProjectConfigurationProvider.html
I think there are more such examples.
>JS02
Method isApplicable() switch the responsibility of providing an algorithm 
of generating unique id from J2EE Profiler implementor to "MAIN" profiler 
implementor ( there are should be real profiling code which implements 
real profiler logic ).  This is why I have chosen this approach.
There is a number ways of choosing Provider when collision appears. It is not
a problem. But provider has no bulletproof algorithm for getId() method .
If we care only about absence of problem in NB then getId() is perfect. 
But provider implementor has a problem .
From my point of view such ID should be generated by NB IDE which knows
all available profilers on its start. So it is able to assign unique ID for 
each Provider. Method getId() requires provider to implement such algorithm with
no option.
At the end : I disagree with you. But I will change isApplicable() to getId().
>JS04
I agree that getDefaultServerSettings is not good.
But I don't like also getServerSettings() name.
I don't see other good suggestion with method name. This is the problem.
Once again: I will change it to getServerSettings().
>JS05
Port number is not hardcoded.
I didn't say this. I say that agentID and port number magically appeared in the
J2EE profiler implementation. It means that they are statefully  migrated from 
J2EEProfilerProjectType class ( not sure about exact name ) where they was 
"generated" to the logically unrelated J2EEProfilerSPI .
Let's forget about NB Profiler. But let's imaging other profiler which works 
in the same way but allows several sessions simultaneously . It is not able to start listen the same port number . So it requires to use another port.
Java agruments where port number is used are passed to Settings. Without 
settings modification new profiling session could not be started ( port is 
already occupied ).
>JS06
There is information which is related to project and J2EE server which should 
be started in profile mode : this is at least agent ID.
As I said before : currently it is badly designed stateful approach access
to agent ID which was generated for Project. 
Suggested approach allows generally pass such information to the J2EE profiling 
session in a good way.
Once again : I disagree with you to remove Properties and other added methods 
from Settings. But I will do it.

(In reply to comment #14)
> >JS01
> I disagree.
> Here is examples which is already in NB API :
> http://bits.netbeans.org/dev/javadoc/org-netbeans-modules-refactoring-api/org/netbeans/modules/refactoring/spi/ProgressProvider.html
> http://bits.netbeans.org/dev/javadoc/org-netbeans-modules-projectapi/org/netbeans/spi/project/ProjectConfigurationProvider.html
> I think there are more such examples.
I'm sure I'll find some as well. We should try to do it better, not to repeat the mistakes. BTW ProjectConfigurationProvider.getActiveConfiguration() / getConfigurations() perfectly follows the pattern.

> >JS02
> Method isApplicable() switch the responsibility of providing an algorithm 
> of generating unique id from J2EE Profiler implementor to "MAIN" profiler 
> implementor ( there are should be real profiling code which implements 
> real profiler logic ).  This is why I have chosen this approach.
> There is a number ways of choosing Provider when collision appears. It is not
> a problem. But provider has no bulletproof algorithm for getId() method .
> If we care only about absence of problem in NB then getId() is perfect. 
> But provider implementor has a problem .
> From my point of view such ID should be generated by NB IDE which knows
> all available profilers on its start. So it is able to assign unique ID for 
> each Provider. Method getId() requires provider to implement such algorithm
> with
> no option.
> At the end : I disagree with you. But I will change isApplicable() to getId().
Just changing the name doesn't make any sense, it won't make the API better. Let's revisit this one based on the reference implementation.
BTW I don't think that the collisions are a real problem - I assume most in most of the cases there will be just one profiler registered. And I don't believe there will be more than two profilers registered. Whereas the API should handle any number of registered providers, we should't screw it for just a hypothetical situations.

> >JS05
> Port number is not hardcoded.
> I didn't say this. I say that agentID and port number magically appeared in the
I'm answering "port number now is hardcoded in JVM arguments" - have I misunderstood it?
> J2EE profiler implementation. It means that they are statefully  migrated from 
> J2EEProfilerProjectType class ( not sure about exact name ) where they was 
> "generated" to the logically unrelated J2EEProfilerSPI .
> Let's forget about NB Profiler. But let's imaging other profiler which works 
> in the same way but allows several sessions simultaneously . It is not able to
> start listen the same port number . So it requires to use another port.
> Java agruments where port number is used are passed to Settings. Without 
> settings modification new profiling session could not be started ( port is 
> already occupied ).
I'm afraid we're still not on the same page. I'm trying to tell you that agent port and agent ID are NB profiler-specific implementation details, it's not a requirement for a profiler to use port and/or id. A different profiler may use for example a memory mapped/config file for communication with the agent, it will configure the server with a path to the file instead of port/id. Any required profiler-specific settings are just transparently passed to the profiled server using ProfilerServerSettings.jvmArgs, the API/framework should not know anything about the details. It's profiler's responsibility to assign a unique ID/port number or whatever settings for each profiling session, do not try to solve this by the framework.

> >JS06
> There is information which is related to project and J2EE server which should 
> be started in profile mode : this is at least agent ID.
Agent id has no relation to project. It's a NB profiler-specific way to correctly handle running agents and recognize an externally started agent. BTW agent ID is also generated from Settings tab - there's no project context.

> As I said before : currently it is badly designed stateful approach access
> to agent ID which was generated for Project. 
> Suggested approach allows generally pass such information to the J2EE profiling 
> session in a good way.
You're right, currently this is badly designed/implemented. But it's a problem of the NB profiler implementation and it should be fixed there. Currently I don't see any reason to handle this by the API. We'll find out more with the reference implementation.
> Once again : I disagree with you to remove Properties and other added methods 
> from Settings. But I will do it.
I'm afraid this approach simply doesn't work. The API should contain just the methods required for the intended features. If the implementation isn't possible without the methods, let's introduce them. Based on the documentation you've provided so far the methods are not used and thus not needed.

>JS01
It's a mistake in your opinion.
In my opinion it's not a mistake.
So where is the truth ?
If you sure that this is mistake please provide a link to rules which 
mandatory require your approach.

Other items are still opinions.
Let's wait a prototype.

Y01 This issue is marked as API_REVIEW, not API_REVIEW_FAST, as such you shall call an inception meeting according to http://wiki.netbeans.org/APIReviewSteps

Y02 This issue used to have target milestone 6.9 (I've just changed to next). Do you really mean that? It may be hard to finish both rounds of review soon enough (given that feature freeze deadline has already passed), I am afraid.

Y03 To have a meeting you need to prepare inception materials. The usual form of inception materials is based on answers to subset of arch.xml questions (with attribute when="init", see http://wiki.netbeans.org/APIDevelopment#Writing_the_documentation). Code patches are not really important at this stage of review. It is more important to cover usecases, high level architecture issues, narrow scope of the work, define time estimates, testability goals, etc.

Y04 As soon as you have the materials ready, publish them here, select four voting reviewers and call for the conf-call.

In case you have some organizational questions, feel free to email me. I will try my best to answer (I should know the answers, I am the reviewers chair, but ...).

>Y
You are right . It's not 6.9 milestone.
It's my fault.

I will contact to you about review.
Thanks.

Prototype is committed into http://hg.netbeans.org/prototypes/
repository.
j2ee-profiler-prototype branch.

Created attachment 97448 [details]
Changes in arch.xml respectively proposed API

Please see arch.xml file diff for API review process.

Here is the list of reviewers for proposed API:
Petr Hejl, Vince Kramer, Tomas Hurka, Petr Suchomel

Y11 "register something into global lookup" - this triggers the performance side of me: does that mean that if I have two plugins for two servers installed and I try to profile just one of the servers, classes for both servers will be loaded? That would be important scalability problem. Either register into some other place than default lookup or use declarative registration via @annotation[1]

Y12 Don't needlessly remove API classes. Deprecate them.

Y13 I can't really comment on the usecases being complete or not, that is more a task for API domain experts.

Y14 How the new API will be covered with tests?


[1] Described for example in this page: http://wiki.apidesign.org/wiki/CompileTimeCache

For review, reviewers:
Jarda Tulach
Jiri Sedlacek
I'll be an observer
--Petr

The last: as an addition to current.

JS1:
I agree with Jirka, the naming / design of quoted ProgressProvider interface is a good example of bad naming design. ProgressProvider does not provide anything, it is just interface where you can register listeners, I wish I new why the name like this. I general, I would expect design such XXXProvider with something like XXX XXXProvider.getDefault() or XXXProvider.getXXX()

>Y11 
I agree with you that there is possible performance problem.
But I don't understand what is declarative registration via
@annotation from your link and I don't get you point how it helps here.
The link is about caching "service provider" on compile stage.
ProfilerProvider is pluggable so it could be not available on NB compile stage.
So it needs to be registered somehow directly without annotations.
As I understand runtime annotation processing is not a good way.

Also I don't see a difference between default lookup usage or 
"other place registration" because classes will be loaded anyway 
on "choosing by ID" stage no matter how they are registered .

For avoiding loading big classes I would change enter point from 
ProfilerProvider to ProfilerFactory. This class is responsible only for 
checking against required ID and lazy instantiate profiler classes 
( which could be big ). These factories should be registered via META-INF/services
( or declared as @ServiceProvider which is the same ) and available via default
lookup . Does it work for you? 

>Y12
OK.

>Y14
Do you mean API tests ?
I don't know how API test should be written. 
Could you please provide me information about this ?

Can we agree on reviewers list?
The list I suggested doesn't work as I understand .
So could someone please assign reviewers because my choice has no matter ?
Vince Kraemer is not available now. It will be after April 19.

Could someone also assign a meeting for review ?

Created attachment 97586 [details]
Registry class for profilers

>Y11
I have realized that functionality which allows to identify profilers 
could be moved into J2EE Profiler integration SPI .
Please see attached ProfilerRegistry class .
This class usage solves two problems:
- No need getId() or isApplicable() method in ProfilerProvider.
So Provider don't need to implement identifier logic with possible 
collision problem .
This functionality is given by API.
- Absence of performance problem.
Provider classes will no be loaded until they are identified as 
requested by client.
I have checked that ProfilerRegistry doesn't load the class till it identified.

I will update API description respectively these changes.

Please let me know your opinion on this.

Comment on attachment 97448 [details]
Changes in arch.xml respectively proposed API

diff -r 177e5db417f9 j2eeserver/arch.xml
--- a/j2eeserver/arch.xml	Wed Apr 14 18:11:50 2010 +0400
+++ b/j2eeserver/arch.xml	Mon Apr 19 14:24:52 2010 +0400
@@ -71,9 +71,10 @@
 <answer id="arch-overall">
 
 <p>
-    There are two separate api/spi sets in j2eeserver. The devmodules
+    There are three separate api/spi sets in j2eeserver. The devmodules
     api/spi is for j2ee development modules (web, j2ee app, etc). The
-    plugins api/spi is for server plugins.
+    plugins api/spi is for server plugins. The profiler api/spi is for
+    profiler J2EE integration plugins.
 </p>
 
 <api name="J2EEServer-devmodules" type="export" category="friend" group="java">
@@ -92,6 +93,11 @@
   </ul>
 </api>
     
+<api name="J2EEProfilerAPI" type="export" category="friend" group="java">
+	J2EE Profiler api/spi is an intergation bridge between J2EE servers registered in IDE 
+	and pluggable profiling capability .
+</api>
+
 <api type="export" group="layer" name="PluginRegistration" category="friend" url="@TOP@/org/netbeans/modules/j2ee/deployment/plugins/api/doc-files/plugin-layer-file.html">
     XML layer contract for registration of server plugins and instances that implement 
     optional capabilities of server plugins.  Plugins with
@@ -327,6 +333,77 @@
     </p>
 </usecase>
 
+<usecase id="j2ee-profiler-api" name="J2EE Profiler Plugin">
+	J2EE application profiling capability could be integrated in to IDE.
+	In order to do this plugin needs to implement interface <a href="@TOP@/org/netbeans/modules/j2ee/deployment/profiler/spi/ProfilerProvider.html">
+	ProfilerProvider</a> and register it as ProfilerProvider service in default lookup.
+	It is important to note that J2EE profiler plugin doesn't provide profiling functionality.
+	This plugin just manages J2EE server registered in IDE for profiling purpose.
+	It allows to start/stop J2EE server instance and tracks its state.
+	There are should be also functionality which allows to <b>profile</b>. But this is not covered here.
+	This functionality is separate module.
+</usacese>
+
+<usecase id="profiler-distinguish" name="J2EE Profiler Plugin Identifier">
+	IDE could have several Profiler providers registered. In order to distinguish them based on some id there is 
+	<a href="@TOP@/org/netbeans/modules/j2ee/deployment/profiler/api/ProfilerRegistry.html">ProfilerRegistry</a> class.
+	Method <code>getId()</code> allows to get identifier for profiler provider implementation which could be used
+	for passing futher by client ( into Ant task f.e. ). It allows to request proiling via chosen profiler.
+	Method <code>getProvider()</code> should be used on J2EE server instance side to access chosen J2EE profiler.
+</usacese>
+
+<usecase id="profiler-start" name="Start J2EE server in profiling mode">
+	There are two ways to start J2EE server in profiling mode:
+	<ul>
+		<li>Ant task <code>nbstartprofiledserver</code> </li>
+		<li>"Start in Profile Mode" action for J2EE server instance ( currently this is AWT Action)</li>
+	</ul>
+	The first one uses ant task properties configured based on project. 
+	<a href="@TOP@/org/netbeans/modules/j2ee/deployment/profiler/api/ProfilerServerSettings.html">ProfilerServerSettings</a> class is instantiated 
+	based on this properties . 
+	Method "getDefaultServerSettings()" is used in case of starting J2EE server in profile mode without original project properties 
+	( case when server is started via AWT Action ).
+	<a href="@TOP@/org/netbeans/modules/j2ee/deployment/profiler/spi/ProfilerSession.html">ProfilerSession</a> created by
+	<a href="@TOP@/org/netbeans/modules/j2ee/deployment/profiler/spi/ProfilerProvider.html">ProfilerProvider</a>  is used for managing
+	profiling of J2EE server instance.
+
+	Profiler provider could support singleton/several simultaneous sessions . In order to handle this situation there is a method
+	<code>canConfigure()</code>. User will be asked to stop profiling sessions in case of requested session can't be configured 
+	to start till <code>canConfigure</code> return <code>true</code>.
+
+	New session start could require to modify java arguments in 
+	<a href="@TOP@/org/netbeans/modules/j2ee/deployment/profiler/api/ProfilerServerSettings.html">ProfilerServerSettings</a> 
+	( f.e. network port change , etc. ). This can be done via method <code>configureSession()</code>.
+
+	When session is configured for starting class <a href="@TOP@/org/netbeans/modules/j2ee/deployment/plugins/spi/StartServer">StartServer</a>
+	is used for starting J2EE server in profiling mode. Each J2EE server plugin is responsible for implementing 
+	profiling functionality implementing.
+</usacese>
+
+<usecase id="state-tracking" name="J2EE server state tracking">
+	There is a need to track state of J2EE server instance .
+	In order to do this <a href="@TOP@/org/netbeans/modules/j2ee/deployment/profiler/spi/ProfilerSession.html">ProfilerSession</a> has
+	ability to register listeners.
+</usacese>
+
+<usecase id="stop-profiling" name="J2EE server stop">
+	There are two ways to stop profiling J2EE server:
+	<ul>
+		<li>"Stop" action for J2EE server ( currently AWT Action )</li>
+		<li>Stop profling with stop profiled application option ( currently this is Stop Profile Button )</code>
+	</ul>
+
+	The first one is controlled by J2EE server instance class which has a knowledge about profiling session . There is a session method
+	<code>requestStop()</code>. Implementation of this method is responsible for notifying listeners with <code>STATE_STOPPING</code>
+	profiling state change. Also it can provide some additional clean up code for profiling stop.
+
+	Other way to stop : Profiler stop with profiled application option .  In this case ProfilerSession implelentation should notify 
+	its listeners about state change.
+
+	So in both cases ProfilerSession listeners will be notified by <code>STATE_STOPPING</code> state .
+	J2EE server instance class is registered as ProfilerSession listener . So it is able to stop server carefully .
+</usacese>
+
 </answer>
 <!--         
         <question id="arch-time" when="init">
@@ -751,6 +828,8 @@
 <answer id="lookup-lookup">
 Folder lookup is used to read the definition of plugin (the additional optional 
 functionality registered by the plugin).
+<br/>
+Default lookup is used for access to profiler provider classes.
 </answer>
 <!--
         <question id="lookup-register" when="final" >
@@ -1152,7 +1231,12 @@
 -->
  <answer id="compat-deprecation">
   <p>
-   No change to existing APIs.
+	  Class <a href="@TOP@/org/netbeans/modules/j2ee/deployment/profiler/api/ProfilerSupport.html">ProfilerSupport</a> is deprecated.
+	  It has not been required from very beggining. It is just container for constants which are now are moved into 
+	  <a href="@TOP@/org/netbeans/modules/j2ee/deployment/profiler/spi/ProfilerSession.html">ProfilerSession</a>. It's single static 
+	  method is just duplication of method in 
+	  <a href="@TOP@/org/netbeans/modules/j2ee/deployment/profiler/spi/ProfilerSession.html">ProfilerSession</a>.
+	  Mentioned method has ambiguity becuase of current profiler privider concept ( there is no singleton provider and session ).
   </p>
  </answer>

Created attachment 97606 [details]
Updated arch.xml diff

Re. Y11 - motto: "loading even a single implementation class is too much". It is probably hard to explain this in prose text, rather let's use an example. You mentioned factories, so let's demonstrate the use of annotations to eliminate needless factories and improve scalability. Please follow the same concept as has been done for @AntBasedProjectRegistration: http://hg.netbeans.org/main/rev/2a6a3bb9a45e
There also used to be a factory (called AntBasedProjectType). Many of its implementations were registered in META-INF/services. Instead of doing that everyone is using @AntBasedProjectRegistration now, which leads to more scalable and also simpler code. Please follow similar pattern with profiler registrations.

>Y11
I'm really sorry for my stupidity but I don't understand the 
annotation advance in considered case ( profiler ).
Let me describe my points. Please indicate where I'm wrong.
- Annotations are used on compile time . Annotation processor 
reads annotations and creates layer.xml file as result.
- Annotation doesn't help when IDE is running. Discovering classes via
Lookup works via reading layer.xml files which could be created via 
annotations scan from previous item.
- layer.xml file could be also created without annotation. One need to use
appropriate format of layer.xml for Lookup service registration.
- As result annotation doesn't help to solve service provider discovering 
performance issues . One still need to check service providers registered in 
the lookup. So there is still need to find  provider somehow ( via id f.e. ).
- Annotations help to avoid layer.xml file usage. This is the advantage.

Annotations work very well for ant based project because main purpose of this API
is providing INTERNAL functionality to register various project types for IDE.
It is used widely in NB modules.
But thirdparty plugin needs to use layer.xml direct registration. Because 
such plugin is distributed in binary format . 

J2EE Profiler integration API is oriented on thirdparty plugins.
There is only one internal NB J2EE profiler plugin .
All other possible providers is supposed to be distributed in binary format.
I can change profiler registration contract from default lookup to 
layer.xml . 
I can add special annotation and provide processor which generates layer.xml
file based on this annotation.
But I don't see a reason for this.
It will work  only for single NB J2EE profiler.
And I still need somehow to identify registered profilers.
Proposed ProfilerRegistry class gives required functionality ( even annotation 
approach requires its functionality probably adopted to another lookup ). 
I don't see how it can be improved because provider class should be loaded 
when it is identified . Other providers will not be loaded.

Once again : sorry , but I really don't understand.

>Y11
It seems I have understood the annotations advantage you suggested for
Ant based projects:
1) There is only one class AntBasedGenericType loaded instead of several 
different implementations of AntBasedProjectType.
2) Project implementor doesn't need to write AntBasedProjectType implementation
at all. There is only one requirement : annotate project class with 
AntBasedProjectRegistration.
3) AntBasedProcessor will generate layer.xml file even for thirdparty 
project plugins on compile stage if module was created via NetBeans.
Module build process invokes annotation processor automatically .
So there is no need to have module as part of NB common build .

Now here are things which is not so good with this approach 
from my point of view:
1) There are two classes AntBasedProjectFactorySingleton and AntBasedGenericType
required instead of one AntBasedProjectType implementation .
2) Method AntBasedProjectFactorySingleton.create() is called via reflection 
as I understand . Method AntBasedGenericType.createProject() uses reflection 
call also.
3) AntBasedProjectType interface has method getType() with requirement to 
return unique identifier for implementation class. It is not possible to control
uniqueness of this identifier . One cannot recover situation with two equal
identifiers.

As consequence :
1) Let's "n" be a number of project factories which one needs to use at the time ( not available but really required to load for project creation ). 
The quantity of loaded classes will be "n+2" because AntBasedProjectFactorySingleton and AntBasedGenericType classes also needs to 
be loaded. 
2) There is one reflection call per factory ( "create" method ) and
each project instantiation requires reflection call.
3) It's plugin implementor responsibility to avoid identifier collisions.

Please look at the attached registry class code ( ProfilerRegistry ) .
This is my proposition how to avoid performance problems and identifier 
collisions.
- Methods "getProvider()" loads ONLY provider which is required ( which 
has requested ID ). No other providers will be loaded at all.
- Method "getId()" can be used to ID provider access
( Currently it has wrong argument ProfilerProvider . It should be 
Class<? extends ProfilerProvider> ). This method also doesn't load provider 
classes which has different class. This id is generated by existed framework
and there is no need to require ID access from plugin implementation. 
There are no probable collisions.
- Let's "n" be a number of required profiler provider ( which should 
be loaded ). Only "n" classes will be loaded ( against "n+2" in annotation approach ). Only one reflection call per provider : lookup provider instantiation ( against at least two reflection call in annotation approach ) .

If you still think that I should use annotation approach I will do it.

Thanks for creating the branch Denis. I have just set a build job up, so we can see the most recent javadocs:
http://deadlock.netbeans.org/hudson/job/prototypes-j2ee-profiler/
I hope to find the arch profiler usecases section listed there soon (it does not seem to be listed in the overview now).

Denis, if you want to run some other tasks (like tests), tell me, I can add them. Or contact me by email and I can give you setup privileges for the job.

I am not really following this proposed API, but since I saw some discussion about project.ant come by, I will respond to this subthread:

(In reply to comment #33)
> 1) There is only one class AntBasedGenericType loaded instead of several 
> different implementations of AntBasedProjectType

Correct.

(With cooperation from AntBasedProjectFactorySingleton, the processor could actually have been written to write the registration to a specific path that would be looked up directly based on project.xml#//type; this would mean that just one _instance_ of AntBasedGenericType would need to be loaded, for a smaller additional optimization.)

> 2) Project implementor doesn't need to write AntBasedProjectType implementation

Correct.

> 3) AntBasedProcessor will generate layer.xml file even for thirdparty 
> project plugins on compile stage

Correct.

> if module was created via NetBeans.

Does not matter how it was created; if you run javac on a source file using the annotation, the registration will be created.

> 1) There are two classes AntBasedProjectFactorySingleton and
> AntBasedGenericType
> required instead of one AntBasedProjectType implementation

Since AntBasedProjectType came first, it must be supported for compatibility. If the API were designed from scratch we would probably omit it.

> 2) Method AntBasedProjectFactorySingleton.create() is called via reflection

Yes, this is the case for any instance registered in a layer. Of course this part is hidden from the API.
 
> AntBasedGenericType.createProject() uses reflection call also.

Of course, that is how the implementing module's code is loaded; it could not be done otherwise.

> 3) AntBasedProjectType interface has method getType() with requirement to 
> return unique identifier for implementation class. It is not possible to
> control
> uniqueness of this identifier . One cannot recover situation with two equal
> identifiers.

That has nothing to do with the use of annotations; it was always a requirement that project types be unique. (Using, say, the Project impl class' FQN was not desirable, since the ID is part of user metadata and should not refer strongly to impl details of the project type module.) I don't think this has ever been a problem for anyone.

> 1) Let's "n" be a number of project factories which one needs to use at the
> time ( not available but really required to load for project creation ). 
> The quantity of loaded classes will be "n+2" because
> AntBasedProjectFactorySingleton and AntBasedGenericType classes also needs to 
> be loaded. 

Plus dozens more classes actually implementing the project type functionality. The "+2" part is asymptotically insignificant (especially as these classes are small).

> 2) There is one reflection call per factory ( "create" method ) and
> each project instantiation requires reflection call.

Yes. So? A single reflection call has negligible overhead.

Jesse, thanks for answers .
I don't review project.ant module. I just trying to find a better way 
to do J2EE Profiler integration API . It differs from project.ant API.
So I just compare the annotation approach with project.ant module as example.

Just a few comments:
- Surely "n" and "n+2" are both O(n) . But "n" is less then "n+2". This
has no matter for project.ant where "n" is big. But I don't expect big "n"
for profiler implementation classes.
- Is it really true ?:
>if you run javac on a source file using the annotation, the registration will be created.
I have checked this outside of NB.
It doesn't work. Probably I missed something. But there is no guarantee that
someone other will not miss this.
It is really strange actually. Processor extends LayerGeneratingProcessor which
could be not in classpath on compile time ( if it doesn't require by API ).
I don't see how compiler resolve this problem on loading annotation processor.

(In reply to comment #36)
>> if you run javac on a source file using the annotation,
>> the registration will be created
>
> I have checked this outside of NB.
> It doesn't work.

File a bug report if you can reproduce. It certainly should work, assuming you do not explicitly specify -proc:none or the like.

> Processor extends LayerGeneratingProcessor which
> could be not in classpath on compile time

It has to be in the processorpath or you will get a NoClassDefFoundError during processing and the compile task should fail. (The apisupport harness sets the processorpath to the transitive closure of the classpath for this reason; other build systems such as Maven can do the same.)

The proposed patch does not include a sample of the coding that would need to happen in the server plugins....  I would think that providing a complete demonstration of the utility of the API change would be a good thing before the review is considered complete.  I think others have made a similar request.

Is the April 8, 2010 patch the most up-to-date patch?  It seems like it may be out of date, based on the number of comments this issue has generated....

Vince, there is already prototype of API usage with all necessary changes
on J2EE server plugin .
It is inside http://hg.netbeans.org/prototypes/ repository.
Please see comment #19 for branch name.

I can do a patch for you with all changes included if mentioned repository
doesn't work for you.

(In reply to comment #39)
> Vince, there is already prototype of API usage with all necessary changes
> on J2EE server plugin .
> It is inside http://hg.netbeans.org/prototypes/ repository.
> Please see comment #19 for branch name.
> 
> I can do a patch for you with all changes included if mentioned repository
> doesn't work for you.

yes. please pull an updated patch and attach it OR include step-by-step instructions on how to extract the  patch from the prototype repo.

Created attachment 99781 [details]
Patch

Last patch proposition

API review: http://wiki.netbeans.org/APIReview182772

Is this still active?

No.