Community
Participate
Working Groups
We still register services for the benefit of code that does not benefit from a multi-agent scenario but an agent is always present and those services are obsolete. As long as they are there, it's hard to detect code that makes use of them although it really should not. The services should be removed and all places that uses them should be changed to instead use an agent and obtain the needed API's from there.
Tentatively targeting to 3.6.
Note I started this process in the tests by adding convenience methods on AbstractProvisioningTest: getAgent(), getMetadataRepositoryManager(), getProfileRegistry(), etc. This makes it somewhat easier to handle in test cases and isolates the code so we can change how the services are looked up later. One problem I found while doing the agent work is that there is no sure way to even *find* all the places where people are doing service lookups. This was my main reason for adding SERVICE_NAME constants for every service, so that you can search for references to this field to find all uses of that interface as a service (and also gives a convenient way of documenting what is a service and what isn't). When I did the agent work, I focused on the engine, director, repository managers, touchpoints, and the various command line applications for converting to agent-based services. There are probably a few bugs left in here, but I think the bulk of the remaining work is in components where agent-specific code wasn't a big concern - the console, the tests, and some parts of the UI.
Looking at the repository code (and bug 301900) I think we need to let the repository managers keep a field that points to the agent that provided them and let the repositories point back to their managers. That way, there is always a path to the agent in charge of a repository or repository manager. I will go ahead and make that change unless anyone objects.
Now that we have a dedicated IProvisioningAgent interface I think we should also have dedicated methods for obtaining the services that we know that it provides, i.e. IArtifactRepositoryManager getArtifactRepositoryManager(); IMetadataRepositoryManager getMetadataRepositoryManager(); etc. since it vouches for much nicer code with less type-casts.
Created attachment 158325 [details] Patch removing all references to p2 services (except the IProvisioningAgent) This patch touches a lot of files so I don't intend it to linger very long before I commit. The patch introduces an IProvisioningAgent parameters in a few places and replaces a large number of calls to the ServiceHelper.getService() or to bundleContext.getService() with the corresponding calls to the agent. All tests are green with this patch (this time the UI tests were included ;-) ). If I don't here any objections, I plan to commit this patch later tonight.
(In reply to comment #3) > Looking at the repository code (and bug 301900) I think we need to let the > repository managers keep a field that points to the agent that provided them > and let the repositories point back to their managers. That way, there is > always a path to the agent in charge of a repository or repository manager. > > I will go ahead and make that change unless anyone objects. That sounds fine to me. I was originally trying to keep the repository managers agnostic of p2 agent so they were more "POJO" - clients could just inject the services they need rather than populating an agent with services. For example I thought this would be easier for writing automated tests that were more isolated (inject mocks). However it doesn't scale very well because as soon as you discover a need for another server, you need to add a new setter and change all callers to provide it. Without some kind of injection framework I think you're right that just providing the agent is the best way to go here.
(In reply to comment #4) > Now that we have a dedicated IProvisioningAgent interface I think we should > also have dedicated methods for obtaining the services that we know that it > provides, i.e. > > IArtifactRepositoryManager getArtifactRepositoryManager(); > IMetadataRepositoryManager getMetadataRepositoryManager(); > etc. I don't agree with this. The whole reason for splitting p2 into multiple bundles was that not all parts of p2 are needed in every deployment scenario. Say for example you just have a metadata verification tool, it might just need p2.core, p2.repository, and p2.metadata, but no engine or director. If you add these API dependencies into IProvisioningAgent, which in turn all bundles rely on, you've tied everything together and we might as well put all of p2 into one bundle (or at least all the API). The goal with the provisioning agent was to be a completely scalable and service agnostic container, that had no dependencies on any particular services it provides. Other people can come along on top of p2 and provide additional provisioning-related services to an agent. We could reduce some of the casting overhead using a signature like this though: public <T> T getService(String serviceName);
(In reply to comment #7) > I don't agree with this. The whole reason for splitting p2 into multiple > bundles was that not all parts of p2 are needed in every deployment scenario. Very true. Forget my idea :-) > We could reduce some of the casting overhead using a signature like this > though: > > public <T> T getService(String serviceName); If we're willing to sacrifice the use of the SERVICE_NAME constant and instead rely on the interface, we could do this even nicer: public <T> T getService(Class<T> serviceInterface); That would enable a type check in the getService() method.
I committed the patch.
This broke both PDE Build and UI, bug 302209 is for PDE/UI. I think I've fixed build.
(In reply to comment #10) > This broke both PDE Build and UI Sorry about that. The bug about PDE's use of API (bug 292691) was closed after the merge. Perhaps we should have a similar bug open until all API changes have completed?
(In reply to comment #8) > If we're willing to sacrifice the use of the SERVICE_NAME constant and instead > rely on the interface, we could do this even nicer: > > public <T> T getService(Class<T> serviceInterface); > > That would enable a type check in the getService() method. The only problem with this that comes to mind is that the caller might not have visibility on the class in question. The caller obtaining the service could be some kind of generic broker or proxy that is obtaining the service from someone else. They may not have visibility on Class<T>, but want to get the service to pass it to someone else who does have visibility on that class. Another case that I just hit is that I want to start the garbage collector if it is available, but not put a hard dependency on it. Currently I can add this code in the p2 engine: agent.getService("org.eclipse.equinox.internal.p2.garbagecollector") This will cause the gc to be activated, but I don't even need to reference the service class which I don't import. If the gc service is not available I don't care. Tom, are there other general reasons why API such as BundleContext.getServiceReference() use String rather than Class as the argument?
We could retain the current method using parameter overloading: Object getService(String serviceName) <T> T getService(Class<T> serviceInterface);
(In reply to comment #12) > Tom, are there other general reasons why API such as > BundleContext.getServiceReference() use String rather than Class as the > argument? The reasons you state about not needing visibility to the Class is the main one. But for normal usecases where the client does have visibility to the Class the other method is nice. For OSGi R4.3 we are adding the following generified methods to BundleContext (overloaded like Thomas suggested in comment 13): <S> Collection<ServiceReference<S>> getServiceReferences(Class<S> clazz, String filter) throws InvalidSyntaxException; <S> ServiceReference<S> getServiceReference(Class<S> clazz); <S> S getService(ServiceReference<S> reference); But we still have the String versions that return ServiceReference< ? > types: ServiceReference< ? > getServiceReference(String clazz); ServiceReference< ? >[] getServiceReferences(String clazz, String filter) throws InvalidSyntaxException; So if getService is passed a type of ServiceReference< ? > you still need to cast but if you pass getService a type of ServiceReference<S> then you don't need to cast to type <S>.
Ok, overloading sounds fine to me. I found it nice that I could search for references to one of the SERVICE_NAME fields to find all people using a given service, but I suppose that's the tradeoff for less casting.
I discovered today that the engine was still being registered as an OSGi service in EngineActivator. I have just released the fix for this (remove service registration). This caused several p2 test failures that I have also fixed up.
