@Heiko, I agree(see below), but should this be a JON bug since rebranding is usually productization/rebranding/non-community?

Other than that, agree with descriptions. We should modify the logic and build process to instead retrieve the information from the EAP rebranding contents(https://docspace.corp.redhat.com/docs/DOC-124664),  files and determine product in that way and then only fall back on the JBossProductType logic for legacy plugins and managed resource discovery.  

I believe the JBossProductType.* logic was done because branding steps were not clearly defined.

See also Bug 997606, Bug 998973 and Bug 988894

Just off the top of my head, based on a call with thomas, heiko - perhaps we can do something like this in the XML descriptor

<callbacks>

   <!-- this tells the plugin container that whenever it runs a discovery for a resource type called "AS7 Standalone Server", it should pass the discovery details returned by that type's discovery component to MyDiscoveryCallback. MyDiscoveryCallback can then massage the results, tweeking it as necessary, and returning the discovery details. The MyDiscoveryCallback can opt to leave the discovered resource details alone and return them as-is -->

   <callback type="AS7 Standalone Server" callback="MyDiscoveryCallback" />

   <!-- this is like above, accept it is not type-specific. Instead, it is saying, anytime you are trying to discovery a type defined in my plugin descriptor (or one of my runs-inside types) using this particular discovery component class, then use MyDiscoveryCallback to again possibly massage that discovery class's returned details -->

   <callback discovery="jbossas7.SubsystemDiscovery" callback="MyDiscoveryCallback" />

</callbacks>

I think we may need 2 upcalls

- discovery helper: we pass in the process we found + an untyped map of properties we found (server base dir, server config, .... ) so that the embedded plugin can go in e.g. take the server config and deduct special directories to search for clues (like trying to find specific jars to inspect)

- post-discovery helper, that gets the DiscoveredResource(s) object(s) and can then say "yes that is me" and rewrite it if needed (e.g. set a different resource name) or return null to say "this is not me", where the PC would just take the original value(s).

Both callbacks do not need to be present and we need to make sure that we can grok multiple embedded plugins having such callback(s). Here we need to have rules what happens if more than one plugin claims that it is the chosen one.

(In reply to Heiko W. Rupp from comment #4)
> can grok multiple embedded plugins having such callback(s). Here we need to
> have rules what happens if more than one plugin claims that it is the chosen
> one.

This is going to be difficult because I'm not sure how we guarantee ordering of who gets called first. I think the embedded plugins should only mess with the discovered details if it ensures its only discovering unique details that it, itself, would only know. But I don't know how feasible that is or if there are any edge cases that should allow for us to accept multiple plugins returning different results. I don't even know what the semantics should be - is it that the first embedded plugin wins? The last one? And if the ordering isn't guaranteed, I'm not sure how that isn't arbitrary either (since the first may be last during different runs of the agent).

(In reply to John Mazzitelli from comment #5)
> This is going to be difficult because I'm not sure how we guarantee ordering
> of who gets called first. I think the embedded plugins should only mess with

We could call all the embedded plugins and check if more than one returns non-null and then abort.
It may be overkill, but on the other hand would expect that this could be quickly checked by deploying all plugins and then testing against the respective resources.

I agree that the "modify result" callback should only be offered to that plugin that says "this is my type".

git commit af523918b7e179c15bcae948cb8e7102e5072732

after talking to heiko, we might want to change the callback interface a bit. right now its:

void discoveredResources(Set<DiscoveredResourceDetails> discoveredDetails)

But it might be nicer to have the PC loop over all the details and pass them in one at a time:

boolean discoveredResources(DiscoveredResourceDetails details)

this would allow each callback to handle them one at a time and be able to return "true" if the callback wants to tell the PC "This is me so I handled this, you can stop calling other callbacks" or the PC could even keep going and if more than one callback wants to handle it, the PC could abort.

So I'm leaving this in the ON_DEV state.

So, I'm in the process of changing the code I commited earlier so that the callback method only gets one resource details passed into it (as opposed to the whole set of discovered resources) - this is so each callback has an opportunity to look at each one individually and can tell us about each individual resource if "it's him" or not.

Heiko and I discussed this and thought it a good thing if we abort the discovery if two or more callbacks return true in their callback method because this would be more than one callback claiming that a discovered resource belongs to them.

But I don't think we want to abort the entire discovery - because if more than one resource is discovered at the same time (that is, the set of details returned by the original discovery component has more than one resource) we will still want to keep the other resources that were discovered.

Heiko - we need to think about this some more. What should happen if more than one callback says the resource details belong to them?

As least we should not abort ALL the discovery - we should just remove the offending resource details from the discovered set of resources but continue with the rest normally.

Are we sure we should abort? Or should we just log a warning and continue, thus allowing more than one callback to change the details? I don't know what to do - I'm leaning towards removing (i.e. aborting) those offending details, but I'm questioning if there may actually be a use case where we could have two or more callbacks that want to adjust discovered details? What if we had a callback implementation whose job is to just modify all resource descriptions, but another callback whose job is to modify all plugin configuration properties? Are we going to shoot ourselves in the foot if we force an abort but later find out there is a use case where it is valid to have more than one callback adjust the discovered details?

git commit to master : b763d74

IMHO, logging a warning instead of aborting discovery is a better option because it doesn't narrow the possibilities for plugin developers and still allows our primary use case.

Otherwise I looked at the changes and they look good.

We still miss one feature though, because we need to allow plugin developers to veto some resources: for example the Infinispan plugin needs to disable the Infinispan subsystem included in the as7 plugin. Should this be addressed in another BZ?

(In reply to Thomas Segismont from comment #10)
> IMHO, logging a warning instead of aborting discovery is a better option
> because it doesn't narrow the possibilities for plugin developers and still
> allows our primary use case.

Yeah, that's what I questioned. And in fact, my uncertainty was so strong that I added a "backdoor" to turn that off. If you start the agent with that sysprop rhq.agent.discovery-callbacks.never-abort set to true, we do what you suggest - just log something but continue.

> We still miss one feature though, because we need to allow plugin developers
> to veto some resources: for example the Infinispan plugin needs to disable
> the Infinispan subsystem included in the as7 plugin. Should this be
> addressed in another BZ?

This could be easily addressed. We could add a new exception to the plugin API and if a callback determined some resource should be vetoed, the callback should throw that exception. The PC will catch that exception and immediately throw away the discovered details (it would be the same thing that is done when multiple callbacks process the details - but this would be a way for a callback to explicitly tell the PC to throw away the details).

I'll put this back ON_DEV and implement that. Very easy to do.

(In reply to John Mazzitelli from comment #11)
> This could be easily addressed. We could add a new exception to the plugin
> API and if a callback determined some resource should be vetoed, the
> callback should throw that exception. The PC will catch that exception and
> immediately throw away the discovered details (it would be the same thing
> that is done when multiple callbacks process the details - but this would be
> a way for a callback to explicitly tell the PC to throw away the details).

How about a new enum constant 'VETOED'  in addition to 'PROCESSED' and 'UNPROCESSED' instead of an exception?

added VETO enum. added integration test to confirm it works.

git commit to master: 3606bc232276f0a41ded2d646b7651f614fe8283

NOTE: I disabled the test DiscoveryCallbackVetoTest.testDiscoveryCallbacks because something is causing our arquillian setup to deploy another plugin from another test and its breaking this one. I'm trying to find the problem, but for now, disabling the test just so I can get this committed to master so its in the next GA. the test passes if I enable it and run it individually, so the test is functional. It just can't work in the full test suite.

git commit: 2b7ee84

i fixed the test, setting the priority seemed to magically get the other tests' plugins to not get deployed.

Changing the title, as this now applies to all plugins.

Initial wiki page is here https://docs.jboss.org/author/display/RHQ/Discovery+Callbacks

Bulk closing now that 4.10 is out.

If you think an issue is not resolved, please open a new BZ and link to the existing one.