Why do you say remove was unsupported before? I don't see it in the class's impl. It just inherits from Vector and so remove is supported by inheritance. Since the class is public, someone may have called it.
Even if we change the class to impl List, period, we'll break back-compat, just because of the synchronization Vector offers. If anyone out there relies on that, it's a problem. On one hand, the best way would be is to impl Collection, as then someone will be able to use Collections.synchronizedCollection if one needs it, or call toArray etc. But Collection does not have a get(index) method, which might be required and useful ... All in all, I don't feel like SegmentInfos is a true collection (even though its Javadoc starts with "a collection ...". It adds lots of segments related methods. The collection's ones are really get and iterator? So maybe we should just impl Iterable and expose whatever API we feel is necessary? Back-compat wise, if we change anything in this class's extension/implements details, we break it. Unless the folks here don't think we should go to great lengths w/ this class, and do whatever changes we dim are necessary, even at the cost of breaking back-compat. And I'd vote that whether with this class or the new one, we mark it as @lucene.internal. Shai On Sun, Feb 28, 2010 at 2:49 PM, Uwe Schindler <u...@thetaphi.de> wrote: > Hi Shai, > > > > I forgot to mention: Iterable is always a good idea. E.g. during my 3.0 > generification, I made “BooleanQuery implements Iterable<BooleanClause>” and > so on. That makes look the code nice J. Also other classes got this > interface in Lucene. Also adding j.io.Closeable everywhere was a good idea. > > > > Uwe > > > > ----- > > Uwe Schindler > > H.-H.-Meier-Allee 63, D-28213 Bremen > > http://www.thetaphi.de > > eMail: u...@thetaphi.de > > > > *From:* Shai Erera [mailto:ser...@gmail.com] > *Sent:* Sunday, February 28, 2010 1:38 PM > > *To:* java-dev@lucene.apache.org > *Subject:* Re: SegmentInfos extends Vector > > > > I would rather avoid implementing List .. we should implement Iterable for > sure, but I'd like to keep the API open either iterating in-order or getting > a particular SegmentInfo. Another thing, I haven't seen anywhere that remove > is called. In general I don't like to impl an interface just to throw UOE > everywhere ... > > I will open an issue. I usually investigate the code first before I open an > issue. Also, what about back-compat? Are we even allowed to change that > class? If not, then we can deprecate it and introduce a new one ... > > Shai > > On Sun, Feb 28, 2010 at 2:25 PM, Uwe Schindler <u...@thetaphi.de> wrote: > > I think you should open an issue! I like this refactoring, maybe we can > still let it implement List<SegmentInfo> but only deprecated and most > methods should throw UOE. Just keep get() and so on. > > > > ----- > > Uwe Schindler > > H.-H.-Meier-Allee 63, D-28213 Bremen > > http://www.thetaphi.de > > eMail: u...@thetaphi.de > > > > *From:* Shai Erera [mailto:ser...@gmail.com] > *Sent:* Sunday, February 28, 2010 1:20 PM > > > *To:* java-dev@lucene.apache.org > > *Subject:* Re: SegmentInfos extends Vector > > > > Yes that's what I've been thinking as well - SegmentInfos should have a > segments-related API, not a List related. Whether the infos inside are kept > in a Map, List, Collection or array is an implementation detail. In fact, I > have a code which uses the API and could really benefit from a Map-like > interface, but perhaps other code needs things ordered (which is why we can > keep a TreeMap inside, or LinkedHahsMap). That's a great example to why it > should have its own API. > > The Lucene code usually calls SegmentInfos.info(int), but some places call > get(int) (which is inherited from Vector). That's bad. > > SegmentInfos is public, though it's tagged with @lucene.experimental. I > think it should be tagged with @lucene.internal as there's nothing > experimental about it? > > I don't mind doing the refactoring. Not sure how this will affect > back-compat (is it acceptable for this classs?). I've touched SegmentInfos > in LUCENE-2289, so I'll wait for someone to pick it up first, so that I > don't work on it in parallel. > > Thanks, > Shai > > On Sun, Feb 28, 2010 at 1:37 PM, Uwe Schindler <u...@thetaphi.de> wrote: > > I think this is historically. I have seen this in my big 3.0 generification > patches, too. But I did not wanted to change it as Vector has other > allocation schema than ArrayList. But maybe we should simply change it, it’s > a package-private class, right? > > > > But in general subclassing those implementations is not the best thing you > can do. In general the class should extend Object or something else and just > have final field of type List<…>. Exposing the whole API of List to the > outside is bad. > > > > +1 to refactor this class (and don’t let it extend a Collections class). > > > > ----- > > Uwe Schindler > > H.-H.-Meier-Allee 63, D-28213 Bremen > > http://www.thetaphi.de > > eMail: u...@thetaphi.de > > > > *From:* Shai Erera [mailto:ser...@gmail.com] > *Sent:* Sunday, February 28, 2010 12:33 PM > *To:* java-dev@lucene.apache.org > *Subject:* SegmentInfos extends Vector > > > > Hi > > What's the reason SegmentInfos extends Vector rather than say ArrayList? Do > we need the synchronization around it which Vector provides? > > Shai > > > > >
