Ahh ok, the semantics confuse me. I thought of experimental as "we're not yet sure how this thingy will evolve or what it'll turn in to - so feel free to play w/ it but don't count on the API or behavior too much". While 'internal' is more like "we need this public for Lucene internal usage, but we don't commit to its API".
I guess SegmentInfos is somewhere in the middle. I.e., there's nothing experimental about it, but it's not public for just Lucene use ... I'll avoid suggesting another tag name :). I'm fine w/ experimental then. I'll work on it soon. Shai On Sun, Feb 28, 2010 at 6:48 PM, Michael McCandless < luc...@mikemccandless.com> wrote: > This class is @lucene.experimental, so we are free to break it. +1 to > not "extends Vector". > > I don't think we should change to @lucene.internal.... since the > thinking is apps outside Lucene should be able to introspect and see > segment structure in the index. Ie we made this API public so people > outside could call it, but it's experimental so we are free to break > things. > > Mike > > On Sun, Feb 28, 2010 at 10:41 AM, Shai Erera <ser...@gmail.com> wrote: > > 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 > >> > >> > >> > >> > >> > >> > >> > >> > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: java-dev-unsubscr...@lucene.apache.org > For additional commands, e-mail: java-dev-h...@lucene.apache.org > >
