lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Christoph Goller <gol...@detego-software.de>
Subject Re: File timestamps / subclassing of IndexReader
Date Thu, 20 Nov 2003 10:20:35 GMT
Doug Cutting schrieb:
> Christoph Goller wrote:
> 
>> Sorry for the delay. I was busy with other things and it took some
>> time to fix the problems that Doug mentioned.
> 
> 
> No problem.  Thanks so much for working on this patch.  It looks really 
> good, and solves a real problem.  We can't demand that volunteers work 
> on a schedule!

My contribution is small compared to giving the whole thing to the public.
There are a couple of people and small companies doing
consulting and product development based on Lucene (including myself).
All this would not be possible without your work. Thank you Doug.

> My only concerns now are aesthetic.  You add segmentInfos as a protected 
> field of IndexReader, which is a public class.  This means it will 
> appear in the javadoc.  Yet this field is of type SegmentInfos, a 
> package-private class, which will not be in the javadoc.
> 
> I try hard to make the javadoc only show things which are meant to be a 
> part of the public API.  So the question is, should SegmentInfos be a 
> public class, i.e., do we think that folks might need to reference it in 
> their application code, or should it remain package private?
> 
> My rule is that, if you cannot imagine a reasonable use for something, 
> or if there's an already another public way of doing it, don't make it 
> public.  Keeping more of the implementation private makes it easier to 
> compatibly evolve the implementation.  If they really want, folks can 
> change things in their private copy of the sources.  What we want to 
> avoid is incompatible changes for folks who only use public APIs.  Over 
> the long term, a smaller public API is easier to maintain.
> 
> My preference is that SegmentInfos remain package private.  The easiest 
> way to do this would just be to make the segmentInfos field of 
> IndexReader package private (i.e. with no public/private declaration). 
> The only problem is that then IndexReader would not be correctly 
> subclassable from outside the package.
> 
> Alternately, segmentInfos could be made private, will all access from 
> IndexReader only.  Can you see a way to get things to work correctly 
> this way?

I have to think thoroughly about the whole problem. This will take some time.

> 
> Finally, you add a new constructor to SegmentsReader but the old 
> constructor is no longer used (except by the new constructor).  It would 
> be better to simply replace the existing constructor with a new 
> constructor.  Less code is better.

ok.

> 
> If you are tired of working on this, and/or have no more time, please 
> say so.  If you are unable, I will try to address the above issues myself.
> 

No, I would like to finish that.

 > As far as I can tell, this patch looks good.
 > I have a question, though.  You changed the constructor to
 > SegmentReader to take both SegmentInfos and a SegmentInfo.
 > Is there a need to pass both?
 > Also, in SegmentReader you do:
 >
 > -            directory().touchFile("segments");
 > +            if(segmentInfos != null)
 > +              segmentInfos.write(directory());
 >
 > Can segmentInfos ever be == null at this point?

In some of the test cases SegmentReader is used directly without
getting it via IndexReader.open. In those cases segmentInfos might be
null.

 >
 > I don't have the source code here to check what SegmentInfos'
 > write(...) method does, so I can't tell why it was added here.

It writes the segments file, which now (my patch) contains also the index
version number. The version number is incremented each time the index
is changes by either a IndexWriter or a IndexReader.



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


Mime
View raw message