Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 323888 - Remove unnecessary profile entries from install history page
Summary: Remove unnecessary profile entries from install history page
Status: RESOLVED FIXED
Alias: None
Product: Equinox
Classification: Eclipse Project
Component: p2 (show other bugs)
Version: unspecified   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.7 M5   Edit
Assignee: Dean Roberts CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 332655 333018
Blocks:
  Show dependency tree
 
Reported: 2010-08-28 14:47 EDT by Samuel Wu CLA
Modified: 2011-01-19 12:00 EST (History)
5 users (show)

See Also:
dj.houghton: review+


Attachments
Hide profile states that are marked hidden (10.57 KB, patch)
2011-01-05 14:44 EST, Dean Roberts CLA
no flags Details | Diff
patch (6.51 KB, patch)
2011-01-13 17:19 EST, DJ Houghton CLA
no flags Details | Diff
patch (7.04 KB, patch)
2011-01-18 17:13 EST, DJ Houghton CLA
no flags Details | Diff
patch (6.93 KB, patch)
2011-01-19 12:00 EST, DJ Houghton CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Samuel Wu CLA 2010-08-28 14:47:57 EDT
Build Identifier: Eclipse 3.6.0

When a installation action is performed, there are multiple installation history entried created and they all contain the same information. There should be only one entry for one action. If multiple entries need to be generated, they should be listed as the children nodes of a single entry.

Reproducible: Always

Steps to Reproduce:
1. Start Eclipse SDK and open the installation history
2. The following installation entries are listed and they all contain the same content
7/9/10 12:05:11 PM EDT
7/9/10 12:05:10 PM EDT
6/8/10 11:52:43 AM EDT
6/8/10 11:52:42 AM EDT
6/8/10 11:51:09 AM EDT
3. When we install the features of our own, we get muliple entries generated as well. 
It's very confusion when you choose to revert to the history. You don't know which one to go.
Comment 1 Pascal Rapicault CLA 2010-08-29 10:12:29 EDT
Are you sure that they are exactly the same content? I would expect some sort of timestamp to be changed. These entries likely result from the execution of the reconciler and could probably be deleted by the reconciler itself since it knows that it created those.

This could likely be fixed in a point release.
Comment 2 DJ Houghton CLA 2010-08-30 08:59:02 EDT
This may be a side effect of using the reconciler to install things. I noticed last week while investigating another bug that the bundles.info file is being written multiple times during a reconciler install. We write it out at the end but it also seems to get written out by the Configure phase.
Comment 3 DJ Houghton CLA 2010-09-10 17:07:13 EDT
I was incorrect in my previous comment. Although the bundles.info file may be written out more than once, the profile itself is only written out by the engine at the end of the phases.

The "compare versions" for profiles in the About dialog only compares the installed IUs so that is why you don't see the real changes between those versions. (profile properties are being changed, and profile timestamps as well)

I don't think the reconciler can delete the profiles that it creates because there are real changes here. I believe this bug should be marked as WONTFIX. One could say the profile compare viewer could show changes made to profile properties but it is unclear whether these types of changes are interesting to the user. (and wouldn't just confuse them more)

If you want to do some testing yourself and compare the real changes between versions, look in the p2/org.eclipse.equinox.p2.engine/profileRegistry/SDKProfile.profile/ folder. This is where the profile files are. You can unzip them and compare them as text files. (they are sorted)

Some observations from my testing:
- An Eclipse SDK is shipped with 4 profile versions
- If we start up and shut down 2 more are created
- When we install an IU with the reconciler, the profile is only written once (so no extras)
- Here is a list of changes between versions

0 -> 1: initial empty state

1 -> 2: profile still empty, change the value of roaming from true to false
    <property name='org.eclipse.equinox.p2.roaming' value='true'/>

2 -> 3: install everything (Eclipse SDK - 430 IUs)

3 -> 4: change the value of roaming to true
    <property name='org.eclipse.equinox.p2.roaming' value='false'/>

4 -> 5: change values set by the builder to be paths on the local machine
    <property name='org.eclipse.equinox.p2.installFolder' value='/builds/N201009082000/src/N20100908-2000/p2temp/equinox.p2.build/sdk.install.macosx.cocoa.x86/eclipse'/>
    <property name='org.eclipse.equinox.p2.cache' value='/builds/N201009082000/src/N20100908-2000/p2temp/equinox.p2.build/sdk.install.macosx.cocoa.x86/eclipse'/>

5 -> 6 : set the cacheExtensions property (was unset)

    <property name='org.eclipse.equinox.p2.cache.extensions' value='file:/Users/equinox/Downloads/eclipse/.eclipseextension|file:/Users/equinox/Downloads/eclipse/configuration/org.eclipse.osgi/bundles/80/data/listener_1925729951/'/>
Comment 4 DJ Houghton CLA 2010-09-16 16:42:44 EDT
Updated title to better reflect the request.
Comment 5 Pascal Rapicault CLA 2010-09-18 21:06:20 EDT
For most users showing different properties will just be confusing. Maybe this could be added to the admin UI instead.
Comment 6 John Arthorne CLA 2010-12-10 13:48:34 EST
I think the more interesting approach is avoiding the duplicate entries from appearing at all. Looking in my installation history, I see two entries for each time I did an update (not using reconciler). I don't think the user can reasonable be expected to figure out which one to select when reverting (displaying profile properties in the history view isn't going to help). I'm guessing one is from the update, and the other is from the reconcile on startup after update. A user can't really "revert" a change made by the reconciler anyway, it will just get reapplied on next startup. Perhaps the reconciler can flag profile states it creates so they don't appear in the UI?
Comment 7 Pascal Rapicault CLA 2010-12-13 23:34:15 EST
We could hide, or delete, however it seems to me that we would only hide the side effect. Before going and hiding / deleting / ... I think it worth understanding why we are doing this reconciliation when we just finished an installation.  I suppose that some state ( (some sort of timestamp probably) is not properly persisted and we go on doing the reconciliation on restart.
Also fixing this problem would improve startup performance for this case.
Comment 8 DJ Houghton CLA 2010-12-14 08:48:20 EST
The breakdown of each profile on initial startup of a fresh install is listed in comment #3. Dean is investigating what happens and how many profile versions are created when installing via the dropins after first startup.
Comment 9 John Arthorne CLA 2010-12-14 09:52:31 EST
(In reply to comment #7)
> We could hide, or delete, however it seems to me that we would only hide the
> side effect. Before going and hiding / deleting / ... I think it worth
> understanding why we are doing this reconciliation when we just finished an
> installation.  I suppose that some state ( (some sort of timestamp probably) is
> not properly persisted and we go on doing the reconciliation on restart.
> Also fixing this problem would improve startup performance for this case.

Yep, if we are doing an unnecessary profile change that we can completely remove, that would be even better. However for the case where the reconciler does need to make a change, do you agree with the idea of hiding such states from the history view in the UI? This seems similar to how we prevent the user from uninstalling anything that was installed by the profiler. It feels inconsistent that we prevent uninstall but not a revert that has essentially the same effect. Also, since reconciler changes never add or remove "root" IUs, the history entry in the UI will always appear to be identical to the previous one from the user's perspective.
Comment 10 Pascal Rapicault CLA 2010-12-14 11:21:50 EST
>do you agree with the idea of hiding such states from the history view in the UI?
 Yes
Comment 11 Pascal Rapicault CLA 2010-12-14 11:25:51 EST
Actually, there is a patch in bug #325095 that is about naming the profile entries. I have not reviewed the provided patch but I believe that this is a direction that could likely be suitable to hide profiles.
Comment 12 Dean Roberts CLA 2010-12-14 11:44:48 EST
I took a look at the patch.  Unfortunately it does not contain what I think will be the interesting part of the problem.  Namely, how does the change operation know if the Profile needs to be tagged or not.

I've just started looking into this and there does not appear to be much difference in the ProfileSynchronizer path between a restart occuring after a user install operation via the UI and a reconciler operation on the dropins directory.  The former should appear in the profile history, the latter should not.

It may end up being more than that as well.  Really the determining factor on whether a profile should be displayed or not is not who creates the profile, but what the contents of the profile are.

At a simplistic level, if a RollbackProfileElements children are identical to an earlier RollbackProfileElements children, only one of the RollbackProfileElements should be displayed.  Of course, the elemtens children are nested sets of DeferredQueryContentProviders ... indicating expensive operations we wouldn't want to do each time the UI is displayed.

One avenue I'm mulling over is perhaps when a profile is added to the ProfileRegistry, maybe the registry could tag a profile as "hidden" if all is children are identical to a preceeding profile.  I've just started fiddling with this and don't really know if the profile registry really works this way.

Please leap in with an intervention if this sounds really off track.
Comment 13 DJ Houghton CLA 2010-12-14 11:47:43 EST
(In reply to comment #12)
> I've just started looking into this and there does not appear to be much
> difference in the ProfileSynchronizer path between a restart occuring after a
> user install operation via the UI and a reconciler operation on the dropins
> directory.  The former should appear in the profile history, the latter should
> not.
> 
There should be no work for the reconciler to do on a regular startup or a restart after installing something via the UI. The only time the reconciler will do any work is if it detects a change in a timestamp and detects that something needs to be installed/uninstalled. I think any operation initiated by the reconciler could be hidden.
Comment 14 Dean Roberts CLA 2010-12-15 11:17:25 EST
In addition to bundles installed via the drop-ins directory causing an installation history that appears identical to the preceding one (even though it is not)I have found a use case that actually creates installation histories where the only difference in the profile is the time stamp.

The steps to reproduce this are.

1) Have multiple versions of a singleton bundle in the drop-ins directory.
2) Restart so that the most relevant version of the bundle is installed.

From this point on every time the UI is used to install a new bundle, the install will create two profiles.  These two new profiles will differ only in timestamp.

I've created Bug 332655 to track this issue.
Comment 15 Dean Roberts CLA 2010-12-15 16:51:59 EST
To hide profiles created by the reconciler in the UI the reconciler will tag the profile.  One method to tag the profile would be to set a profile property.

However, the UI currently populates the profile list based on profile file name only.  It does not parse or even open the profile files until a particular profile is selected in the UI.

There are two strategies for persisting the the hide/don't hide information and I would like to get comments on which way people think we should go.

1) Encode the hide/no-hide information in the file name.  This has the advantage of being as fast as the current implementation.  It will also be simple to implement.  However, it is not particularly flexible should we want efficient ways to access arbitrary profile properties in the future.  I suggest that we extend the naming convention with a bitmask which would give some ability to easily add extra bits of data in the future.  A TouchPoint implementation could be written to convert a users existing profiles to the new naming convention so that upon upgrade a users existing profiles would benefit from the new UI hiding scheme.

2) Since profiles are essentially static, we would save all profile property data in a separate cache which could be accessed quicker than parsing the entire profile file.  Conceptually this is a cleaner approach and would give us quicker access to any profile properties which may be helpful in general.  The downside is it would be slower than simply reading the file names and the implementation somewhat more complex as we would have to write caching code, reconciliation code and retrofit the IProfile implementation with some lazy loading code.

I personally prefer option #1 but would like to hear from others.
Comment 16 Dean Roberts CLA 2010-12-15 17:01:54 EST
We have identified two scenarios that create profiles that appear in the UI to be identical.  The bug relating to profile properties for non-existant IUs, and the reconciler creating profiles which should not show up in the UI since the user can not uninstall them.

We have plans that could address these two scenarios but arguably the root issue is that the details pane in the UI is making decisions on which information about an update to show (top level IUs only, no property changes, etc.).  

We can make the argument that given this, the UI should also be making decisions about which profiles to list in the profile list using the same criteria.  Namely, the UI would parse each profile file compare it to a previous profile and make a decision on if the profile should be included in the list or not.  Decisions like "the profile contains only property changes, don't show it".  "The profile does not contain any top level IU changes, don't show it".  There are likely others.  As well, given two identical appearing profiles, a decision about which to hide and which to show needs to be made, and this may change based on the nature of the profiles.  For instance a reconciler generated change would likely show the root profile and not the reconciler change profile.  But a collection of property change only profiles would likely show the most recent property change profile.

Clearly, such an approach would have to use caching so that the profile files would not be read each time.

Does anybody see this as a valid path forward or do you think we should continue to search for and address the particular scenarios that are injecting duplicate profiles?
Comment 17 DJ Houghton CLA 2010-12-15 17:44:50 EST
(In reply to comment #15)
> 
> 2) Since profiles are essentially static, we would save all profile property
> data in a separate cache which could be accessed quicker than parsing the
> entire profile file.  Conceptually this is a cleaner approach and would give us
> quicker access to any profile properties which may be helpful in general.  The
> downside is it would be slower than simply reading the file names and the
> implementation somewhat more complex as we would have to write caching code,
> reconciliation code and retrofit the IProfile implementation with some lazy
> loading code.
> 

I prefer Option 2 where we keep a cache in a separate file; modifying the filename seems more like a hack and doesn't offer much in the form of expansion should we have another case come up. A particular profile version is a static thing so once we read it, we shouldn't have to read it again. Also since we are using a SAX parser we could short-circuit the parsing to bail after reading the properties that we are interested in.

(In reply to comment #16)
> Does anybody see this as a valid path forward or do you think we should
> continue to search for and address the particular scenarios that are injecting
> duplicate profiles?
>
I think we should fix the problem where we have identical profiles except for the timestamp (bug 332655) and also mark reconciler-created profiles with a property (or something) so they can be hidden.

After completing both those steps, we should re-evaluate the comparison mechanism that the UI uses but realistically I don't see us needing to pursue changing it. After fixing the above 2 problems I believe we should only have real IU-related changes in the profile. (other than perhaps some of the ones listed in comment #3 (which actually might be removed from the list because of the "caused by the reconciler" flag)).
Comment 18 Pascal Rapicault CLA 2010-12-16 07:39:09 EST
> 2) Since profiles are essentially static, we would save all profile property
> data in a separate cache which could be accessed quicker than parsing the
> entire profile file.  Conceptually this is a cleaner approach and would give us
> quicker access to any profile properties which may be helpful in general.  The
> downside is it would be slower than simply reading the file names and the
> implementation somewhat more complex as we would have to write caching code,
> reconciliation code and retrofit the IProfile implementation with some lazy
> loading code.
   I would vote for #2 but with a variation. The variation consists in storing a new properties file next to each profile (e.g. 123.description) rather than storing it in a uber table. The problem with the uber table is that it is less clear for ppl that the profile is referenced in there, and as such deleting a profile manually from the registry becomes a bit more annoying. 
  The other advantage of the description file is that it can later be used to carry more interesting information about what happened.
  To be fast, profiles that need to be hidden, the name of the description file could be 123-HIDDEN.
Comment 19 Dean Roberts CLA 2010-12-16 08:07:17 EST
(In reply to comment #18)
> > 2) Since profiles are essentially static, we would save all profile property
> > data in a separate cache which could be accessed quicker than parsing the
> > entire profile file.  Conceptually this is a cleaner approach and would give us
> > quicker access to any profile properties which may be helpful in general.  The
> > downside is it would be slower than simply reading the file names and the
> > implementation somewhat more complex as we would have to write caching code,
> > reconciliation code and retrofit the IProfile implementation with some lazy
> > loading code.
>    I would vote for #2 but with a variation. The variation consists in storing
> a new properties file next to each profile (e.g. 123.description) rather than
> storing it in a uber table. The problem with the uber table is that it is less
> clear for ppl that the profile is referenced in there, and as such deleting a
> profile manually from the registry becomes a bit more annoying. 
>   The other advantage of the description file is that it can later be used to
> carry more interesting information about what happened.
>   To be fast, profiles that need to be hidden, the name of the description file
> could be 123-HIDDEN.

I would agree that if we use the caching mechanism we would go the route of having a matching "description" file for each profile.  However, I think the suggestion that we encode the hidden/not hidden flag in the name of the description file actually says more in defense of option 1.  How can we argue that it is OK to encode hidden/unhidden UI state in the name of the description file and not just go ahead and support it in the name of the profile file itself.

In fact, the decision was already made that the names of the profile files are encoding specific information needed by the first pane of the history UI, namely the time stamp.  It really doesn't feel like much of a stretch that if that UI pane also needs to hide profiles it be allowed to encode that information in the file name, along with the timestamp information it already encodes.  I think option #2 adds a lot of complexity, and performance degredation for very little gain.  We go from a simple directory scan and string parse to scanning two types of files, open a file, half parsing it, writing another file.  Scan directories and see if we have new files that don't have generated description files etc.  Not to mention simply doubling the number of profile files in the directory.
Comment 20 John Arthorne CLA 2010-12-16 12:00:49 EST
We should keep in mind that both the reconciler and the UI go entirely through the IProfileRegistry API. They don't know anything about filenames, and the UI can't be grovelling over the file system shape by itself and building a cache. It can only access the IProfileRegistry API. Whether our default registry implementation uses file names, property files, etc, seems like an implementation detail that isn't too important and we can easily change in the future.

So another way to approach the problem is, what API do we need? For example this is the most simple addition:

1) public long[] listProfileTimestamps(String id, boolean includeHidden);

Where "hidden" is a well known profile property that the registry knows about.

Somewhat more complicated but more flexible would be:

2) public long[] listProfileTimestamps(String id, Filter filter);

This would return all profiles whose properties match the given LDAP filter. This way the profile registry knows nothing about "hidden" and the API is a bit more general.

Now API 1) could be implemented by playing tricks with the filename, but API 2) would require something like Pascal said: the profile registry writes out the profile in two files: A Java properties file containing the profile properties, and the usual compressed XML file for the IUs. This isn't a cache that requires reconciling, it's just a change in how the profile registry persists profile states (much like we changed from raw XML to Gzipped files in 3.5).

So answering the API question first would help drive what the default implementation of that API looks like. From this perspective I kind of like API 2). It would allow future enhancements like giving a "name" or "tag" to a profile state and allow lookup of profile states by that name.
Comment 21 Dean Roberts CLA 2011-01-05 14:44:50 EST
Created attachment 186115 [details]
Hide profile states that are marked hidden

This patch contains a "real" UI change in ProfileSnapshot that will hide any profile states that have the state meta data property IProfile.PROP_HIDDEN set.

This patch requires that patch for Bug 333018

This patch also includes the quick and dirty hack to ProfileSynchronizer so that profiles added via the drop-ins directory are marked as hidden that we talked about in the call before Christmas.  The hack relies on the fact that the Profile held on to by the ProfileSynchronizer gets updated with the new timestamp when it is applied.  This is likely insufficient for production use.  Instead I suspect a change is in order so that the perform methods can return the updated Profile in the status.

To see this patch in action, you can install this patch and the one from Bug 333018.  Place a new plug-in in the drop-ins directory and after restart see that the installation state is not listed in the Installation History page
Comment 22 DJ Houghton CLA 2011-01-13 17:19:33 EST
Created attachment 186794 [details]
patch

Updated patch. (untested)
Comment 23 DJ Houghton CLA 2011-01-18 17:13:19 EST
Created attachment 187054 [details]
patch
Comment 24 DJ Houghton CLA 2011-01-19 12:00:09 EST
Created attachment 187134 [details]
patch
Comment 25 DJ Houghton CLA 2011-01-19 12:00:59 EST
Patch released.