Community
Participate
Working Groups
Created attachment 183810 [details] Patch to consolidate p2 interaction The current target provisioning code uses p2 profiles to back the various IU containers but this is done in an inconsistent way. there are a number of issues with how added IUs are managed and how the profile is synchronized with the target definition. I'm attaching a patch which does the following in order to make the profile management more consistent and better performing. The bulk of the changes are in IUBudnleContainer and P2TargetUtils. - consolidate the management of p2 "things" on the P2TargetUtils class. The key goal here is to minimize exposure of p2 and to ensure cache coherence. getProfile always makes sure that the profile and the target are in sync. From there you are good to go. - Add a sequence number to targets. Each time a target is changed in a way that affects what it represents, its sequence number is incremented. This allows "caches" (e.g., profiles) to check their consistency easily. I have done preliminary testing of these changes and things are looking good. I'll do more testing in the near future. I'd appreciate feedback on the direction. I backed off moving the *include* things from the IUBundleContainer to the TargetDef. Having them on the container actually gives us some flexibility down the road. This also minimizes churn on the current .target files. We do NOT have to update the file version number.
Please note, the current patch has a couple problems I discovered in testing last night. It would still be interesting to get feedback on the direction but please do NOT commit these changes. More later.
Created attachment 184066 [details] updated patch This new patch is pretty close to what we want. Here is a summary of the changes (including those from Comment 0) - consolidate the management of p2 "things" on the P2TargetUtils class. The key goal here is to minimize exposure of p2 and to ensure cache coherence. Most of this was moving the code from IUBundleContainer to P2TargetUtils. - IUBundleContainers now have an instance of P2TargetUtils (fSynchronizer) to which they delegate management and coordination of the backing profile. The containers receive a callback when the synchronizer detects that something has changed. This allows them to update their IU, Bundle and Feature caches. - Added a lifecycle to containers so they now know when they are associated with a target. This allows containers to coordinate their inclusion settings. its a noop for other containers - Add a sequence number to targets. Each time a target is changed in a way that affects what it represents, its sequence number is incremented. This allows "caches" (e.g., profiles) to check their consistency easily. - updated the caching model to allow containers to push their cache updates into the AbstractBundleContainer rather than wait to be called. This is required because the list under the covers can change without the container changing (if using the planner). - updated the test cases as needed. All pass. - IUBundleContainers are now immutable (other than removeIU). The constructor takes a bitmask int for the include options rather than allowing them to be set. Previously it was really only valid to set them at construction time. This just makes that clearer as the lifecycle around containers is quite confusing. - IUBundleContainer resolution now takes into account the repos from all other IUBundleContainers. This addresses various usability issues. It does change the list of bundles (and features) returned from an individual container however. It is possible now for container A to report bundle X where X actually comes from the repo associated with container B. In practice this is what you want because adding the roots in A brought in X. Note that this is only an issues when using the planner. Pending topics - P2TargetUtils should really be called P2TargetSynchronizer but I didn't want to clutter the patch with a pretty wide reaching name change. - some more test cases
I'm reviewing the latest patch and so far so good. I've added/fixed some comments. When using obscure parameters like 'sequence number' and 'flags' it is important that the documentation says what the allowed values are and their purpose.
Created attachment 184185 [details] Patch with my updates This patch adds fixes a few details I saw when looking at the code. However, I am having problems getting targets with multiple containers to work. The second or third containers end up with no bundles (on the location tab or the content tab) even though the download did appear to be running correctly. Also, with the change to store the sequence number, will there be any problems with old files that don't have a sequence number? It seemed like things would be ok, but I would appreciate your thoughts.
(In reply to comment #4) > However, I am having problems getting targets with multiple containers to work. > The second or third containers end up with no bundles (on the location tab or > the content tab) even though the download did appear to be running correctly. I have the following in my console Profile does not contain any bundles or artifacts were missing
Created attachment 184195 [details] example target that works for me (In reply to comment #4) > However, I am having problems getting targets with multiple containers to work. > The second or third containers end up with no bundles (on the location tab or > the content tab) even though the download did appear to be running correctly. Odd. Please attach a .target that exhibits this problem. Looking at how the code works, there would have to be a profile that resolves ok etc but then the slicing for the container would have to fail for some reason. IUBundleContainer#cacheBundles() is the place that this is done. Note that this slicing is done regardless of the provisioning mode. It is the one that figures out which of the profile bundles are attributed to this particular container. It may be something particular about your .target and the slicing options being used. Note, I have attached a target that works for me in all combinations of include required/include source. notice that the number of bundles attributed to the various containers changes with each change in settings. > Also, with the change to store the sequence number, will there be any problems > with old files that don't have a sequence number? It seemed like things would > be ok, but I would appreciate your thoughts. Worst case without a sequence number the target would have to be re-resolved from scratch. P2TargetUtils#checkProfile assumes that if the .target sequence and profile sequence numbers are the same then everything is good. If they are not the same it does a deeper check to see if the .target and the .profile actually represent the same thing. If they do we are happy. If not, the profile is rebuilt from scratch. > I have the following in my console > Profile does not contain any bundles or artifacts were missing You must have debugging turned on. That is coming from IUBundleContainer#cacheBundles() when it figures out that the container defined no bundles. In the current model that is not expected. The first paras of this comment talk about what could be going on.
Created attachment 184217 [details] Some additional tweaks Here is a new patch (based on Curtis' latest) that improves the following: - progress reporting in the resolve* call chain through IUBundleContainer and P2TargetUtils. Changed to use SubMonitors, cleaned up counts and added more cancel checks. Likely more improvements could be had. - There was an NPE bug in the Update operation - Update and remove were not triggering re-resolve so the UI did not update Some remaining topics: - In the typical long running case all the hard work (downloading etc) is done while resolving the bundles for the first IU container. However, the progress ticks are allocated over the containers and bundle and feature resolution. As a result the user sees a relatively short progress bar and then bink, done. - The Update seems to always mark the .target file as dirty regardless of whether it actually changed. I can't find where the marking is happening so dunno how to fix it.
Marking for M5, while this is getting close, this is a risky change for the last week of M4. Issues I have noticed: 1) Pressing the remove button when a root IU is selected does not cause the locations group to refresh properly. A refresh is run, but the IU is not removed. Saving, closing and reopening the editor seems to workaround it. 2) With two p2 containers in the location and two root IUs in one container, select one IU and remove it. First problem (1) happens, but after closing and reopening I find that the wrong IU was removed. I haven't seen my previous issue anymore. Perhaps I had stale information in the profile.
Example target file I am seeing problems with. The second location originally had RCP and Releng tools. When I removed RCP IU, it removed Releng tools for me. <?xml version="1.0" encoding="UTF-8" standalone="no"?> <?pde version="3.6"?> <target name="test" sequenceNumber="17"> <locations> <location includeAllPlatforms="false" includeMode="slicer" includeSource="true" type="InstallableUnit"> <unit id="org.eclipse.platform.sdk" version="3.7.0.I20101130-0900"/> <repository location="http://fullmoon.ottawa.ibm.com/updates/3.7-I-builds/"/> </location> <location includeAllPlatforms="false" includeMode="slicer" includeSource="true" type="InstallableUnit"> <unit id="org.eclipse.rcp.feature.group" version="3.7.0.v20101115-9FB-Fp8Fr3P05bnSCyQ_nSS"/> <repository location="http://fullmoon.ottawa.ibm.com/updates/3.7-I-builds/"/> </location> </locations> </target> In the end, the issues I am seeing are not major and the patch actually fixes several outstanding issues. We will consider putting in for M4. Another minor thing I noticed is that the containers like to reorder themselves every time I modify the IUs in them.
Created attachment 184319 [details] Latest patch that fixes the remove problem I found the problem with remove. Embarassing but I had the boolean logic backwards and was removing everything BUT the thing that you asked to be removed. Sigh. Easy one line fix in the removeInstallableUnit() method. This new patch fixes just that problem. The missing UI refresh problem is not happening for me. It refreshes every time. Perhaps you were seeing that issue with the previous version of the patch? There was a refresh change that I put in to help both Update and Remove but that was soooo, "yesterday" :-) There was mention as well of a container switching order problem. I have seen that happen but it seems variable. It happened at least once on adding an IU to a container and also when removing an IU (using the Remove button). But it does not happen all the time for me. One thought I had was that perhaps it is because the two containers in your sample target have the same repo location. Another thought is that there is actually a race condition between the UI doing the update and the resolve jobs happening in the background. with some breakpoint in I noticed that there are some intermediate states where the UI "appears" half updated. the containers have no content and may or may not be in the other order. Sometimes this all works out by the time thing are done and other times the container orders end up different in the UI. In fact, I found a case where the order shown in the UI is different from that in the actual TargetDefinition. In fact, I even found a case where the BundleContainerContentProvider.getElements was returning things in the right order but the UI was showing them in a different order. So far I have never seen a case where the Target itself has the order wrong. Dunno, this one is out of my league and I suspect little to do with these changes. I'll continue checking various scenarios but right now don't see any show stoppers.
The viewer does have a comparator set, so the ordering in the UI can easily differ from the content provider. You are probably right that the weird ordering changes are because I have two of the same repository, the ordering could easily change based on which one was resolved with bundles. All my testing so far today has gone perfectly. Resolving works great, the profile is getting used correctly, even when multiple p2 containers are there, the content tab is working correctly, etc. etc. I wish we didn't have to add a new method to ITargetDefinition. The purpose of the sequence number is not going to be clear to most people using targets. I don't believe anyone is looking at providing their own target definition implementation so API wise we are ok. I think we should rename the method to getCurrentSequenceNumber() or getSequenceNumber() to make it more in line with the other methods. I can do this before committing today if there end up being no more changes.
The sequence number is there as a fast way of telling if a target has changed. The only place it is needed is in P2TargetUtils.checkProfile() where we compare the target to the profile to see if they match. The profile is, in effect, a cache of the target so we need to know when to invalidate. Check profile has a backup plan that does a deeper comparison but this was thought to be costly. I have changed things a little so that synchronize() (the only caller of checkProfile()) is used less often. It is still called twice per IUcontainer in a target for each resolve. That's not that often and is typically part of a bigger operation so we might be able to live without the sequence number. If you want to cut the API turmoil for M4 we can remove currentSequenceNumber() from ITargetDefinition and just cast to TargetDefinition pending the investigation of completely removing the sequence number. Let me know what you want me to do.
I was a little hasty with my "its not called all that often" statement. I did a quick trial by commenting out the sequence number check in checkProfile(). That is the only place we actually care about the value. Modulo a separate bug I found, things seem to work fine. But, it turns out that checkProfile is called in isResolved() and that is in turn called quite frequently. For example, from getChildren()/hasChildren() in the UI. The cost still is not tragic but it is in the UI thread whereas the normal resolve sequence is executed on some job. So, we still have the option but it is not quite as rosy as originally suggested.
Not sure if you have done any changes but while investigating the sequence number stuff I found a cut and paste error in P2TargetUtils. Please update the method as follows. private void setPlanProperties(IProvisioningPlan plan, ITargetDefinition definition, String mode) { plan.setProfileProperty(PROP_PROVISION_MODE, mode); plan.setProfileProperty(PROP_ALL_ENVIRONMENTS, Boolean.toString(getIncludeAllEnvironments())); plan.setProfileProperty(PROP_AUTO_INCLUDE_SOURCE, Boolean.toString(getIncludeSource())); plan.setProfileProperty(PROP_SEQUENCE_NUMBER, Integer.toString(definition.currentSequenceNumber())); } Note the change on the PROP_ALL_ENVIRONMENTS line. it used to set the value to getIncludeSource()!
Created attachment 184374 [details] Patch with API removed This patch, which I plan to commit after lunch, does the following: 1) Removes the sequence number from ITargetDefinition. The method is accessed by casting to TargetDefinition. We still do this for other options that don't belong as API like UI mode. 2) Renames the method to getSequenceNumber() 3) Fixes setPlanProperties (comment 14) 4) Marks ITargetPlatformService as @noimplement, which should have been done before.
Applied my patch to HEAD. If any major problems show up during test, we will reopen and revert. For minor issues, please open new reports. Thanks Jeff.
woo hoo! Thank you Curtis for all your time and help with this. I will look at removing actual use of sequence numbers and open another bug if that seems valuable.
