Bug 332314

Comment 1 Jeff McAffer 2010-12-10 15:32:48 EST

Created attachment 184996 [details]
partial removal of singleton references

This patch is a first cut at removing references to the default ProvisioningUI
et al.


Remaining references to ProvisioningUI.getDefaultUI()...

How to hook up RevertProfilePage and InstalledSoftware Paget to the UI or agent
- Not sure where these are instantiated and if the ProvisioningUI can be
injected there.

ElementWrapping
- Seems to mostly be needed around the Session to check enablement of repos

ProvUIProvisioningListener
- need the UI operation runner to check if the batch is over.

PreloadingRepositoryHandler and subtypes
- These command handlers need to find out what UI they are bound to.  Not sure
where they are created (via the registry?) but somehow they need the UI
injected.


Remaining references to ProvUIActivator.getDefault().getProvisioningUI()
- There are a few around the UI model

Comment 2 Jeff McAffer 2010-12-14 17:04:56 EST

Created attachment 185179 [details]
updated patch 

This new patch should catch the vast majority of cases where the default ProvisioningUI is being used.  We have not looked particularly at the Admin UI though that would be interesting. I've done some initial testing of these changes and shown that for normal PDE workflows we are not using the default provisioning ui (great) and that the basic IDE provisioning workflows work.  More testing is required but this is looking pretty good.

The changes are pretty much as described so far and summarized breifly here.
- analyze and remove references to the getDefault() style methods on ProvUIActivator and ProvisioningUI.  MOst of the time there is a UI around that we can use.  A few places methods were modified to pass a UI in if one was not available.

- QueryProvider was isolated just to QueriedElement.  This allowed for the removal of sevaral getQueryProvider methods.

- The wrappers and model structure were analyzed. For the most part model bits will get the UI from their parent structure.  The MetadataRepositoryElement had to be treated a little differently as it is sometimes a root and sometimes not

- added a tracing flag /ui/default that logs whenever the default ProvisiningUI is used.  Initial testing shows that in the PDE usecase the default UI is NOT used.  This is great.

- the test suites have been updated but not run.

Comment 3 Jeff McAffer 2011-01-14 12:14:10 EST

Created attachment 186833 [details]
updated patch for review

Just to be sure everything is up to date, here is an updated patch against HEAD. This should be good for review.  Note that we have not addressed uses in the admin UI or the discovery UI.  I'll open a separate bug for the admin UI in due course.

Comment 4 Jeff McAffer 2011-01-14 12:43:32 EST

Note that  when I was updating from HEAD I noticed that ColocatedRepositoryTracker#validateRepositoryLocation was removed in a recent change.  Not sure why or if that was just some confusion in my diff'ing.  This patch retains the method but it may need to be removed.

Comment 5 Susan McCourt 2011-01-17 16:41:27 EST

(In reply to comment #3)
> Created attachment 186833 [details]
> updated patch for review
> 
> Just to be sure everything is up to date, here is an updated patch against
> HEAD. This should be good for review.  Note that we have not addressed uses in
> the admin UI or the discovery UI.  I'll open a separate bug for the admin UI in
> due course.

There are compile errors in the admin UI when this patch is loaded so we'll need to address before this patch can be released.

(In reply to comment #4)
> Note that  when I was updating from HEAD I noticed that
> ColocatedRepositoryTracker#validateRepositoryLocation was removed in a recent
> change.  Not sure why or if that was just some confusion in my diff'ing.  This
> patch retains the method but it may need to be removed.

It needs to be removed (the implementation of that method was pushed to the abstract class as part of bug 311633.

I've just loaded the patch and am reviewing...

Comment 6 Jeff McAffer 2011-01-18 09:32:29 EST

Created attachment 186999 [details]
patch for changes in admin ui

Indeed you are right re the Admin UI compile errors.  Sorry I missed those changes in the original patch.  This patch contains just the Admin UI changes required so apply both to your workspace.

As for the ColocatedRepositoryTracker method, I removed it in my workspace but am not going to attach a new patch just for that change.  If I should have to regen the patch later that change will be in.

Comment 7 Susan McCourt 2011-01-18 11:46:52 EST

Reviewed both patches.
(Thanks for the trace options!)

Overall, +1 on the change with a couple questions/caveats.

- I'm not sure what the implications are of commenting out the MockQueryProvider in the AbstractQueryTests.   They still pass, but I'd feel better if Ian could take a look and confirm that they are still testing what needs to be tested.

- Is there any contract breakage resulting from the change of the declaration of the repository tracker service?  The old declaration said it provided the interface RepositoryTracker.  The new one provides IAgentServiceFactory.  As I read the code, it looks like it's doing the right thing, it just threw me off to see the interface change.  Can older clients still get the tracker the old way:

(RepositoryTracker) ServiceHelper.getService(bundleContext, RepositoryTracker.class.getName());

I realize that pattern doesn't set up the correct ProvisioningUI, just making sure we aren't introducing an unintended API breakage.

- Don't forget to delete the ColocatedRepositoryTracker method.

I like the cleanup and think it greatly simplifies the code (especially with respect to the query provider), so this is good stuff.  Thanks and sorry for the delay (I never added myself to the bug when we first discussed the changes so I didn't see the patch in December...)

Comment 8 Jeff McAffer 2011-01-18 13:01:02 EST

Created attachment 187028 [details]
consolidated patch with admin ui, tests, core and ui

This is a new consolidated patch that pulls everything together with a couple tweaks as noted below.

(In reply to comment #7)
> - I'm not sure what the implications are of commenting out the
> MockQueryProvider in the AbstractQueryTests.   They still pass, but I'd feel
> better if Ian could take a look and confirm that they are still testing what
> needs to be tested.

Ian and I did talk about this and decided that the tests should not need to do the Mock stuff.  

If that is indeed the final conclusion then the commented out lines should be removed and MockQueryProvider can also be removed as dead code.

> - Is there any contract breakage resulting from the change of the declaration
> of the repository tracker service?  The old declaration said it provided the
> interface RepositoryTracker.  The new one provides IAgentServiceFactory.  As I
> read the code, it looks like it's doing the right thing, it just threw me off
> to see the interface change.  Can older clients still get the tracker the old
> way:
> 
> (RepositoryTracker) ServiceHelper.getService(bundleContext,
> RepositoryTracker.class.getName());
> 
> I realize that pattern doesn't set up the correct ProvisioningUI, just making
> sure we aren't introducing an unintended API breakage.

Its a good question.  I dont think that it was declared that there would be a supplied RepositoryTracker as an OSGi service.  Most of the uses I could find went through ProvisioningUI.getRepositoryTracker().  Having said that, I found one place in the AdminUI and one place in the tests that was using it as an OSGi service. 

Summary: I suspect that there will be very few users of RepositoryTracker and the ones that there are will use the getRepositoryTracker() method. Of course, now that that has been said, some will turn up...   We could in theory setup an OSGi service as before for the default ui but this code would be (I suspect) a bit complicated and perpetuate a model that we don't want.

> - Don't forget to delete the ColocatedRepositoryTracker method.

in the new patch.

Comment 9 Susan McCourt 2011-01-18 13:28:44 EST

> Ian and I did talk about this and decided that the tests should not need to do
> the Mock stuff.  
> 
> If that is indeed the final conclusion then the commented out lines should be
> removed and MockQueryProvider can also be removed as dead code.

I think we should do that, it will be less confusion later, or at least open a bug so it can be done later.

> Its a good question.  I dont think that it was declared that there would be a
> supplied RepositoryTracker as an OSGi service.  Most of the uses I could find
> went through ProvisioningUI.getRepositoryTracker().  Having said that, I found
> one place in the AdminUI and one place in the tests that was using it as an
> OSGi service. 

Hmmm...the more I think about it, the more I think this is kind of broken.  The intention here was an app could plug-in their own tracker if, for example, they didn't want to enforce colocation, or wanted a different way to enumerate the repos (such as hiding some of them, etc.).  Before the patch, the decision that "RepositoryTracker should be ColocatedRepositoryTracker" is made in the component xml, and someone else could override it with a higher service ranking.  As long as it was originally gotten from the registry, then someone could plug in a new one.

The way it is in the patch, choice of that class is hard-coded in the new component.  So I think we do have to fix this problem.  (If we had API tools for service declarations I would imagine this would be flagged as breakage).
What if we instead check the instance of the returned service and if it's ColocatedRepositoryTracker, we set the UI into it.  If it's not, then we presume someone knows what they are doing.  I know this is ugly but I think it's more in keeping with the spirit of the original service.

If this is getting too complicated to discuss here, find me on IRC equinox-dev...

Comment 10 Jeff McAffer 2011-01-18 13:56:40 EST

(In reply to comment #9)
> > If that is indeed the final conclusion then the commented out lines should be
> > removed and MockQueryProvider can also be removed as dead code.
> I think we should do that, it will be less confusion later, or at least open a
> bug so it can be done later.

Agreed.  After (if) Ian chimes in, I can post a new patch or bug as needed.

> Hmmm...the more I think about it, the more I think this is kind of broken.  The
> intention here was an app could plug-in their own tracker if, for example, they
> didn't want to enforce colocation, or wanted a different way to enumerate the
> repos (such as hiding some of them, etc.).  Before the patch, the decision that
> "RepositoryTracker should be ColocatedRepositoryTracker" is made in the
> component xml, and someone else could override it with a higher service
> ranking.  As long as it was originally gotten from the registry, then someone
> could plug in a new one.

That doesn't work in an multi-agent world.  The pluggability is still there but rather than overriding all trackers for all UIs, here you plug your tracker into your agent using ProvisioningAgent.registerService().  The new DS component is registering a factory to, in effect, create the default services.

> The way it is in the patch, choice of that class is hard-coded in the new
> component.  So I think we do have to fix this problem.  (If we had API tools
> for service declarations I would imagine this would be flagged as breakage).
> What if we instead check the instance of the returned service and if it's
> ColocatedRepositoryTracker, we set the UI into it.  If it's not, then we
> presume someone knows what they are doing.  I know this is ugly but I think
> it's more in keeping with the spirit of the original service.

I don't think this is necessary give one's ability to call agent.registerService().  There should really be no one accessing OSGi services for p2 function (except the Agent itself). All services should be accessed through the agent.

Comment 11 Jeff McAffer 2011-01-18 19:39:25 EST

John A and I talked about this briefly and figure that the change is more like something we missed in the introduction of agents and not a significant API change. People who are using the RepositoryTracker should be using the getRepositoryTracker method.

Comment 12 Susan McCourt 2011-01-19 12:09:24 EST

(In reply to comment #11)
> John A and I talked about this briefly and figure that the change is more like
> something we missed in the introduction of agents and not a significant API
> change. People who are using the RepositoryTracker should be using the
> getRepositoryTracker method.

I'm fine with that.
That makes me wonder if one should be using the agent to get the UI's Policy, then.  (There is no state that depends on the agent now, but someday?)  Maybe that's another bug for another day.

+1

Comment 13 Jeff McAffer 2011-01-19 12:20:42 EST

As long as UI folks go to the right ProvisioningUI and get things then we should be fine.  In the future we can link the policy into the agent if needed.

Comment 14 Jeff McAffer 2011-01-23 13:26:20 EST

Susan, in comment 12 you put a +1 but the patch has not been committed.  Are there outstanding issues?  If we can still squeak this in for M5 that would be ideal so we can get more feedback.

Comment 15 Jeff McAffer 2011-01-23 13:45:00 EST

Created attachment 187385 [details]
new patch adapting to changes in HEAD

there was one conflicting change in HEAD in ProfileSynchronizer so I updated the patch to resolve the conflict.  Everything else is the same.

Comment 16 Jeff McAffer 2011-01-24 05:18:40 EST

Getting this in M5 may be less important because actual exercising of this possibility will not happen until the PDE change to use the non-default ProvisioningUI will not go in.  So M5 will just validate that things keep working.  Would be useful but is not worth a fire drill.  Your call.

Comment 17 Susan McCourt 2011-01-24 12:48:57 EST

(In reply to comment #16)
> Getting this in M5 may be less important because actual exercising of this
> possibility will not happen until the PDE change to use the non-default
> ProvisioningUI will not go in.  So M5 will just validate that things keep
> working.  Would be useful but is not worth a fire drill.  Your call.

I just assumed you would commit and that you were asking me for review?

Comment 18 DJ Houghton 2011-01-28 15:32:42 EST

It sounds like everything is done here so I've released the code so we can close this.

Comment 19 Jeff McAffer 2011-01-28 17:19:05 EST

wow, aren't you the helpful one.  Thanks DJ.  I was waiting or M5 to be in the bag before releasing.  Thanks.