Ok agreed. I'll do some code investigation and then open an issue. I think that back-compat with this class should not be a (big) problem ... but then - I always think that :).
Shai On Sun, Feb 28, 2010 at 4:04 PM, Uwe Schindler <u...@thetaphi.de> wrote: > Hi Shai, > > > > I am only the Generics Police but not the Generics Homeland Security and > also not the Backwards Homeland Security J I think if we break backwards, > lets break it complete and remove the “extends Vector”. And then let’s make > the Iterator/Iterable/Collection unmodifiable. That would get a big +1 from > my side. > > > > ----- > > 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 2:22 PM > > *To:* java-dev@lucene.apache.org > *Subject:* Re: SegmentInfos extends Vector > > > > Ok so just that I'm cleared - unmodifiable you mean for iteration only > right? > > And .. do you agree then to refactor the class, or prefer to keep it like > that? If you agree, then we need to think if we do that by introducing a new > class, or modify the existing one breaking back-compat. A new class is > problematic since that will lead to a series of deprecations throughout the > code. So I prefer modifying the current one. > > DM - I've traced Vector.remove all the way back to 1.3, and AbstractList > exists since 1.2 (so it's javadocs states), so I think remove has been > around always. > > Shai > > On Sun, Feb 28, 2010 at 3:14 PM, Uwe Schindler <u...@thetaphi.de> wrote: > > I meant it was supported by the API, but if you called the modification > methods of SegmentInfos you may have corrupted the contents. So implementing > List<?> or Collection<?> just throwing UOE is fine, as modifying in > Collections can disabled by that exception, the docs state that. > > > > But you are right, it does not make real sense. With backwards > compatibility I think of plug-in compatibility, not behavior compatibility. > If we want to keep behavior compatibility, we must extend Vector J and > allow all modifications. > > > > So implementing a non-modifiable Collection/List may be the best. But > that’s only my opinion. > > > > ----- > > 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 2:04 PM > > > *To:* java-dev@lucene.apache.org > *Subject:* Re: SegmentInfos extends Vector > > > > 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 > > > > > > > > >
