Bug 293340 - [api][repository] Review of the repository package
Summary: [api][repository] Review of the repository package
Status: RESOLVED FIXED
Alias: None
Product: Equinox
Classification: Eclipse Project
Component: p2 (show other bugs)
Version: 3.6   Edit
Hardware: PC Mac OS X - Carbon (unsup.)
: P3 normal (vote)
Target Milestone: 3.6 M6   Edit
Assignee: John Arthorne CLA
QA Contact:
URL:
Whiteboard:
Keywords: api
Depends on: 296280
Blocks:
  Show dependency tree
 
Reported: 2009-10-26 13:14 EDT by Pascal Rapicault CLA
Modified: 2010-03-11 14:53 EST (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Pascal Rapicault CLA 2009-10-26 13:14:33 EDT
- Do we want to have IMR and IAR into the same package?
- Is IStateful really meant to be API?
- IRepository review the properties on it and probably clean them up.
   - do we want the IDs of the simple repos to be there and API?
- RepositoryEvent, can we use EventAdmin, how is this multiplexed?
- Should RepositoryCreationException extends ProvisionException is it used?
- IArtifactComparator, ArtifactComparatorFactory, should not that be in repo tools?
- ProcessingStepDescriptor needs to be in the repository package
- Should the package name be p2.repository.metadata instead of p2.metadata.repository?
Comment 1 Andrew Niefer CLA 2009-11-27 16:37:02 EST
I have done the following in org.eclipse.equinox.p2.repository:

org.eclipse.equinox.p2.repository
  - IRepository
  - ICompositeRepository
  - IRepositoryManager
org.eclipse.equinox.p2.repository.metadata
  - IMetadataRepository
  - IMetadataRepositoryManager
org.eclipse.equinox.p2.repository.artifact
  - IArtifactRepository
  - IArtifactRepositoryManager
  - IFileArtifactRepository
  - IArtifactRequest
  - IArtifactDescriptor
  - ArtifactKeyQuery
  - ArtifactDescriptorQuery
  - ProcessingStepDescriptor
org.eclipse.equinox.p2.repository.artifact.spi
  - ArtifactDescriptor


> IArtifactComparator, ArtifactComparatorFactory, should not that be in repo
> tools?
I agree that this should be moved to repo.tools, it is used in two places:
1) Mirroring
  - This could be moved to repository.tools.  We currently have two MirroringApplication classes, one from repository.tools and the other from artifact.repository.  The artifact.repository MirroringApplication was the original and I believe is replaced by the repository.tools and should be removed.
 
2) CompositeArtifactRepository#validate
  - I would suggest this can also be moved into repository.tools where the validate task and composite repo application lives.

> IStateful, RepositoryEvent, RepositoryCreationException
these still need to be looked at
Comment 2 Andrew Niefer CLA 2009-12-09 16:30:54 EST
Both metadata and artifact mirroring have now been consolidated to the
p2.repository.tools bundle.

The artifact comparator stuff still needs to move the the repo.tools, but there
are some issues around composite repositories that need to be addressed.  See
bug 296280
Comment 3 John Arthorne CLA 2010-01-11 13:48:26 EST
I'm working on cleanup of repository API, in particular repository SPI.
Comment 4 John Arthorne CLA 2010-03-05 17:25:53 EST
I did another pass over the repository API today, and found some strange things that I am clearing up:

 - Currently IRepository has several set methods for name/description/provider. These are only ever called internally by repository implementations, so I am moving them to be protected methods on AbstractRepository rather than API for all clients of IRepository.  

 - For several repository attributes we have both getter methods, and property keys defined. It's not clear to a client if IRepository.getName() is equivalent to IRepository.getProperty(IRepository.PROP_NAME). Currently they are not equivalent but it feels like they should be. This would be a big change at this point, so I am leaving the get* methods on IRepository, and we could possibly reconcile with the properties in the future so that getName() is just a convenience to access the property.

 - Most fields on AbstractRepository were protected and therefore API, which prevents us doing things like the above in the future (storing repository name/description/provider as properties). To give us some flexibility I have made all fields private and instead added protected get/set methods for subclasses to use.

This is a bigger change than I had intended, but the net effect is just some eliminated API that we could always add back if needed in the future. There were no affected clients outside the p2 code itself.
Comment 5 John Arthorne CLA 2010-03-05 17:34:55 EST
I have released the above changes to HEAD but I will leave this open for remaining javadoc cleanup
Comment 6 Pascal Rapicault CLA 2010-03-08 23:07:21 EST
I just noticed that the I*RepositoryManager interfaces are marked as not implement. Is this expected?
Comment 7 John Arthorne CLA 2010-03-09 09:09:52 EST
(In reply to comment #6)
> I just noticed that the I*RepositoryManager interfaces are marked as not
> implement. Is this expected?

Yes, but I noticed this isn't consistent with other p2 service interfaces so I'll open a more general bug to discuss this.
Comment 8 John Arthorne CLA 2010-03-11 14:53:18 EST
Marking fixed. There is still a bit of missing javadoc but the repository API is largely complete.