Bug 244628 - [api] [repository] artifact repos do not support bulk remove
Summary: [api] [repository] artifact repos do not support bulk remove
Status: RESOLVED FIXED
Alias: None
Product: Equinox
Classification: Eclipse Project
Component: p2 (show other bugs)
Version: 3.4   Edit
Hardware: PC Windows Vista
: P3 normal (vote)
Target Milestone: 3.7 M3   Edit
Assignee: DJ Houghton CLA
QA Contact:
URL:
Whiteboard:
Keywords: api, performance
: 184538 238669 (view as bug list)
Depends on:
Blocks: 313953 321057
  Show dependency tree
 
Reported: 2008-08-19 17:32 EDT by Jeff McAffer CLA
Modified: 2010-09-16 12:15 EDT (History)
6 users (show)

See Also:


Attachments
Added removeDescriptors(IArtifactDescriptors[]) for SimpleArtifactRepository (3.56 KB, patch)
2008-11-07 14:41 EST, Andrew Cattle CLA
no flags Details | Diff
patch (15.55 KB, patch)
2010-08-18 14:01 EDT, DJ Houghton CLA
no flags Details | Diff
patch (15.93 KB, patch)
2010-08-26 08:55 EDT, 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 Jeff McAffer CLA 2008-08-19 17:32:16 EDT
for performance reasons we have bulk add operations on metadata and artifact repos.  We also have a bulk remove on metadata repos.  There is no such operation (other than removeAll) on artifact repos.  This would be very handy in the RepositroyListener for example.
Comment 1 Andrew Cattle CLA 2008-11-07 14:41:27 EST
Created attachment 117357 [details]
Added removeDescriptors(IArtifactDescriptors[]) for SimpleArtifactRepository

The actual implementation is logically identical to that of addDescriptors(IArtifactDescriptors[]) except with "add" replaced by "remove"

I also included a test case I ran on it to verify it works properly.

Because it's a variation on addDescriptors, implementing a removeDescriptors shouldn't be too hard if we decide to promote this to API.
Comment 2 Pascal Rapicault CLA 2008-11-28 15:27:10 EST
Could you please work with Simon and/or John on that. Thx.
Comment 3 Pascal Rapicault CLA 2009-04-29 21:57:13 EDT
*** Bug 238669 has been marked as a duplicate of this bug. ***
Comment 4 Simon Kaegi CLA 2009-07-07 15:23:02 EDT
I'm upgrading this and targetting it.
I just ran a performance test that uses dropins where we end up removing 4000 artifacts from a simple artifact repo and the time taken was over 7 minutes. In 3.4 we used a technique where we re-created the artifact repos after doing removal in an in memory list this resulted in one write instead of "n" writes. That approach is no longer easy to do as the publishing now occurs deeper down in the publisher however we need to find a good alternative.
Comment 5 Pascal Rapicault CLA 2009-08-25 22:49:21 EDT
*** Bug 184538 has been marked as a duplicate of this bug. ***
Comment 6 Pascal Rapicault CLA 2009-10-20 13:23:20 EDT
Ian, you may want to take a look at that in the context of bug #274025.
Comment 7 Ian Bull CLA 2009-10-29 12:49:48 EDT
bug #274025 will provide a solution to this.  Do we still want an explicit remove(IArtifactDescriptor[] desc) method?
Comment 8 John Arthorne CLA 2009-10-29 14:02:55 EDT
(In reply to comment #7)
> bug #274025 will provide a solution to this.  Do we still want an explicit
> remove(IArtifactDescriptor[] desc) method?

The remove case seems much easier because we can delete both the artifact and the index entry in the same call. I think it would be useful to have such a method and fairly simple to implement.
Comment 9 DJ Houghton CLA 2010-08-18 14:01:13 EDT
Created attachment 176923 [details]
patch

Here is a patch against the R36_maintenance branch which adds new API to IArtifactRepository.

John, can you verify that I've done the right thing with the version numbers here? I want to make sure we don't run into problems with people requiring bundles and version ranges, etc.

This is what I've done:
- incremented the minor version of p2.repository since we added API (2.0 -> 2.1)
- incremented the service version of the bundles of all the implementors of the new API
- changed the requires range for p2.engine from [2.0, 2.1) to [2.0, 2.2)
Comment 10 Pascal Rapicault CLA 2010-08-18 18:05:25 EDT
I quickly went through the code and it looks good, but I wonder if we want to introduce a new API in a point release.
Comment 11 DJ Houghton CLA 2010-08-19 07:03:49 EDT
I'm inclined to think the same way. There were a couple of bugs blocked by this but they have been worked around in 3.6. I'm going to bump this to 3.7.
Comment 12 John Arthorne CLA 2010-08-20 10:39:31 EDT
(In reply to comment #9)
> John, can you verify that I've done the right thing with the version numbers
> here? I want to make sure we don't run into problems with people requiring
> bundles and version ranges, etc.

Looks good to me. I also agree on bumping to 3.7 since there is new API here.
Comment 13 DJ Houghton CLA 2010-08-26 08:55:16 EDT
Created attachment 177524 [details]
patch
Comment 14 DJ Houghton CLA 2010-08-26 08:56:27 EDT
Patch released to HEAD.
Closing.
Comment 15 DJ Houghton CLA 2010-09-16 12:15:21 EDT
There was a problem with tagging HEAD so this fix won't appear in integration builds until the first build after 3.7 M2.