lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael McCandless <luc...@mikemccandless.com>
Subject Re: IndexReader.listCommits to return a List
Date Wed, 29 Sep 2010 09:05:46 GMT
On Wed, Sep 29, 2010 at 3:22 AM, Shai Erera <serera@gmail.com> wrote:
> Hi
>
> IndexReader.listCommits returns a Collection, which not only does not define
> and guarantee any ordering between the commits, but also makes it very hard
> to know what is the latest commit. And we have some inconsistency, IMO, in
> how we return the commits today, in the different API:
>
> IndexDeletionPolicy receives a List<IndexCommit> and not Collection.
> Furthermore, it relies on the commits being ordered oldest -> latest, and
> it's somewhat documented.
>
> listCommits, to my surprise, returns the latest commit as the first element
> of the Collection, and the rest of the commits are unsorted. Having seen IDP
> relying on the commits to be sorted, I thought that listCommits sorts them
> too, but was surprised to find out that the latest one appears first.
>
> The implementation already returns an ArrayList, and I wonder why do we need
> the Collection abstraction? Why not returning a List, or even a LinkedList,
> or an array - anything that entails ordering between them. And preferably
> something which will allow me to access first/last really quickly, and
> direct access can be a bonus for all I care.
>
> And we should also document the ordering they are returned, and better make
> it consistent w/ IDP's assumptions. It's a static method, so no one can
> break by extending it. And committing to some ordering should be problematic
> - we already do that with IDPs.
>
> The only problem is that we cannot change the signature of the method, so
> I'm ok w/ either break back-compat and document, or deprecate and create a
> getCommits (or a better name).

+1 to specialize IndexReader.listCommits to List<IndexCommit>, ordered
by increasing generation.

I think we should just break & advertise it (even in 3.x) -- this is a
very advanced API and those users using it can handle the change.

> While we're at it, it'd be great if we can consolidate ReaderCommit and
> CommitPoint. The latter implements Comparable and supports deletion of
> IndexCommits, while the former doesn't do both. Is there any reason why it
> cannot support delete()?

ReaderCommit is DirReader's [private] impl of IndexCommit while
CommitPoint is IndexFileDeleter's private impl, right?

I agree ReaderCommit should impl comparable -- in fact, shouldn't we
move "impls Comparable" up into IndexCommit?  It just compares the
gen, which is always available.

But always supporting delete is trickier, though I think possible and
very desirable?  You'd have to obtain the write lock, create an IFD
instance, forward the delete request(s) to the right commits, and ask
IFD to commit to actually do the deletion.

Mike

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org


Mime
View raw message