lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Hoss Man (JIRA)" <>
Subject [jira] Commented: (LUCENE-743) IndexReader.reopen()
Date Sun, 07 Oct 2007 18:23:50 GMT


Hoss Man commented on LUCENE-743:

Okay, read the patch.  I'm on board with the need for Clonable now ... it's all about isolation.
 if "r.reopen(false) == r" then the client is responsible for recognizing that it's got the
same instance and can make the judgement call about reusing the instance or cloning depending
on it's needs ... internally in things like MultiReader we have to assume we need a clone
for isolation.

Questions and comments...

1. CompoundFileReader, FieldsReader, IndexReader, and BitVector all have clone methods where
they silently catch and ignore CloneNotSupportedExceptions from super.clone()... if we don't
expect the exception to ever happen, we should just let the exception propogate up the stack
(there is no down side to declaring that clone() throws CloneNotSupportedException).  If we
think the exception might happen, but it's not a big deal if it does and we can ignore it,
then we should put a comment in the catch block to that effect.  i'm not clear which are the
cases here.

2. i don't remember if the old patches had this issue as well, but having "return new FilterIndexReader(reopened);"
in FilterIndexReader doesn't really help anyone using FilterIndexReader -- they're going to
want an instance of their own subclass.  to meet the contract of FilterIndexReader, we should
implement all "abstract" methods and delegate to the inner reader - so in theory do don't
need a new instance of FIlterIndexReader, we could just return in.reopen(closeold) ... my
gut says it would be better to leave the method out entirely so that the default impl which
throws UnSupOpEx is used --- that way subclasses who want to use reopen *must* define their
own (instead of getting confused when their filtering logic stops working after they reopen
for the first time)

3. instead of having an isClonable() method, why don't we just remove the "implements Clonable"
declaration from IndexReader and put it on the concrete subclasses -- then use "instanceof
Cloneable" to determine if something is clonable?  ... for that matter, the only place isCloneSupported
is really used is in IndexReader.clone where an exception is thrown if Clone is not  supported
... subclasses which know they are Clonable don't need this from the base class, subclasses
which don't implement Clonable but are used in combination with core classes whose reopen
method requires it could be skiped by the caller (since they don't implement Clonable) ..

4. javadocs frequently refer to "reopened successfully" and "refreshed successfully" when
what it really means is "reopen() returned a newer/fresher reader" ... this may confuse people
who are use to thinking of "successfully" a "without error"

5. random thought: are their any synchronization issues we're missing here?  I'm wondering
about the case where once thread calls reopen while another thread is updating norms or deleting
docs.  is there any chance for inconsistent state?

> IndexReader.reopen()
> --------------------
>                 Key: LUCENE-743
>                 URL:
>             Project: Lucene - Java
>          Issue Type: Improvement
>          Components: Index
>            Reporter: Otis Gospodnetic
>            Assignee: Michael Busch
>            Priority: Minor
>             Fix For: 2.3
>         Attachments:, lucene-743-take2.patch, lucene-743.patch,
lucene-743.patch, lucene-743.patch,,
> This is Robert Engels' implementation of IndexReader.reopen() functionality, as a set
of 3 new classes (this was easier for him to implement, but should probably be folded into
the core, if this looks good).

This message is automatically generated by JIRA.
You can reply to this email to add a comment to the issue online.

To unsubscribe, e-mail:
For additional commands, e-mail:

View raw message