Return-Path: Delivered-To: apmail-lucene-java-dev-archive@www.apache.org Received: (qmail 58516 invoked from network); 1 Mar 2010 08:18:09 -0000 Received: from unknown (HELO mail.apache.org) (140.211.11.3) by 140.211.11.9 with SMTP; 1 Mar 2010 08:18:09 -0000 Received: (qmail 40797 invoked by uid 500); 28 Feb 2010 13:22:47 -0000 Delivered-To: apmail-lucene-java-dev-archive@lucene.apache.org Received: (qmail 40762 invoked by uid 500); 28 Feb 2010 13:22:47 -0000 Mailing-List: contact java-dev-help@lucene.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: java-dev@lucene.apache.org Delivered-To: mailing list java-dev@lucene.apache.org Received: (qmail 40750 invoked by uid 99); 28 Feb 2010 13:22:47 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Sun, 28 Feb 2010 13:22:47 +0000 X-ASF-Spam-Status: No, hits=2.2 required=10.0 tests=HTML_MESSAGE,SPF_PASS X-Spam-Check-By: apache.org Received-SPF: pass (athena.apache.org: domain of serera@gmail.com designates 209.85.219.221 as permitted sender) Received: from [209.85.219.221] (HELO mail-ew0-f221.google.com) (209.85.219.221) by apache.org (qpsmtpd/0.29) with ESMTP; Sun, 28 Feb 2010 13:22:39 +0000 Received: by ewy21 with SMTP id 21so845150ewy.5 for ; Sun, 28 Feb 2010 05:22:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:in-reply-to:references :date:message-id:subject:from:to:content-type; bh=4sZd5b2PgWSeU+F7pfzQ+kHIAO7qeyYgFax1iQMlMKU=; b=YS9Hf18ZCOtbbfv4kNXxPvecolIfxvqWOmXNjA3rEwiPv83oFvReby/A/a2l2DV+zv qIcmItAf7zpJVhcARqZY/5jrFTqSVq+4Bi38WwMKAPaXyee8femkKmgoFjuwDsYb9Uez vnMiXXOxeRIH/k5IEfwxVfJiXbuhzmo/BjgIc= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type; b=W8YdTu/TXW70sqbx+HK53z4H4enUQXcoqfVTueZykn4OylENuHbjw3ol+PC1/be/a7 fbtanuS6KPXgGToHrD4XmjYGvpQs3F9KWKFfDBry100tIx9QISu5De0gudF0UCcCnXnz UTgdDvgmH9hcr6RUxMWwSlpdjN8L0sSYI3ttc= MIME-Version: 1.0 Received: by 10.213.97.79 with SMTP id k15mr1892441ebn.21.1267363336812; Sun, 28 Feb 2010 05:22:16 -0800 (PST) In-Reply-To: <000301cab878$05e3c540$11ab4fc0$@de> References: <786fde51002280333w484971abx6ebd63ca5cee4bbc@mail.gmail.com> <003c01cab86a$6063fc50$212bf4f0$@de> <786fde51002280420j4772fb04x2ce7abcaaa658043@mail.gmail.com> <000301cab871$199c2840$4cd478c0$@de> <786fde51002280437o774c6cabmed16d0333ca55832@mail.gmail.com> <001601cab874$71f18050$55d480f0$@de> <786fde51002280504x5e002af7ofd43b81d47f74abe@mail.gmail.com> <000301cab878$05e3c540$11ab4fc0$@de> Date: Sun, 28 Feb 2010 15:22:16 +0200 Message-ID: <786fde51002280522i54a5befaje4092ed4d39be2bc@mail.gmail.com> Subject: Re: SegmentInfos extends Vector From: Shai Erera To: java-dev@lucene.apache.org Content-Type: multipart/alternative; boundary=000e0ce005a47dfb2e0480a90569 --000e0ce005a47dfb2e0480a90569 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable 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 ne= w 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 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 implement= ing > 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 compatibilit= y. > 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=92s only my opinion. > > > > ----- > > Uwe Schindler > > H.-H.-Meier-Allee 63, D-28213 Bremen > > http://www.thetaphi.de > > eMail: uwe@thetaphi.de > > > > *From:* Shai Erera [mailto:serera@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, o= r > 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 segmen= ts > related methods. The collection's ones are really get and iterator? So ma= ybe > 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/impleme= nts > 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 ne= w > one, we mark it as @lucene.internal. > > Shai > > On Sun, Feb 28, 2010 at 2:49 PM, Uwe Schindler wrote: > > Hi Shai, > > > > I forgot to mention: Iterable is always a good idea. E.g. during my 3.0 > generification, I made =93BooleanQuery implements Iterable= =94 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 ide= a. > > > > Uwe > > > > ----- > > Uwe Schindler > > H.-H.-Meier-Allee 63, D-28213 Bremen > > http://www.thetaphi.de > > eMail: uwe@thetaphi.de > > > > *From:* Shai Erera [mailto:serera@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 fo= r > sure, but I'd like to keep the API open either iterating in-order or gett= ing > a particular SegmentInfo. Another thing, I haven't seen anywhere that rem= ove > 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 wrote: > > I think you should open an issue! I like this refactoring, maybe we can > still let it implement List 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: uwe@thetaphi.de > > > > *From:* Shai Erera [mailto:serera@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 ke= pt > 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 c= an > keep a TreeMap inside, or LinkedHahsMap). That's a great example to why i= t > should have its own API. > > The Lucene code usually calls SegmentInfos.info(int), but some places cal= l > 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 SegmentInfo= s > 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 wrote: > > I think this is historically. I have seen this in my big 3.0 generificati= on > 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, i= t=92s > a package-private class, right? > > > > But in general subclassing those implementations is not the best thing yo= u > can do. In general the class should extend Object or something else and j= ust > have final field of type List<=85>. Exposing the whole API of List to the > outside is bad. > > > > +1 to refactor this class (and don=92t let it extend a Collections class)= . > > > > ----- > > Uwe Schindler > > H.-H.-Meier-Allee 63, D-28213 Bremen > > http://www.thetaphi.de > > eMail: uwe@thetaphi.de > > > > *From:* Shai Erera [mailto:serera@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 > > > > > > > --000e0ce005a47dfb2e0480a90569 Content-Type: text/html; charset=windows-1252 Content-Transfer-Encoding: quoted-printable
Ok so just that I'm cleared - unmodifiable you mean fo= r iteration only right?

And .. do you agree then to refactor the cla= ss, 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 Abstrac= tList 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 <uwe@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. S= o implementing List<?> or Collection<?> just throwing UOE is fine= , as modifying in Collections can disabled by that exception, the docs state tha= t.

=A0

But you are right, it does not make real sense. With b= ackwards 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.

=A0

So implementing a non-modifiable Collection/List may b= e the best. But that=92s only my opinion.

=A0

-----

Uwe Schindler

H.-H.-Meier-Allee 63, D-28213 Bremen

http://www.thetaphi.de

eMail: uwe@theta= phi.de

=A0

From:= Shai Erera [mailto:serera@gmail.= com]
Sent: Sunday, February 28, 2010 2:04 PM


To: = java-dev@lucene.apache.org
Subject: Re: SegmentInfos extends Vector

=A0

Why do you say remove= was unsupported before? I don't see it in the class's impl. It just inh= erits 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-comp= at, 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 w= ill 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 t= hough its Javadoc starts with "a collection ...". It adds lots of segme= nts 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/imple= ments 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 <<= a href=3D"mailto:uwe@thetaphi.de" target=3D"_blank">uwe@thetaphi.de>= wrote:

Hi Shai,

=A0

I forgot to mention: Iterable is always a good idea. E.g. during my 3.0 generification, I made =93Boolean= Query implements Iterable<BooleanClause>=94 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.

=A0

Uwe

=A0

-----

Uwe Schindler

H.-H.-Meier-Allee 63, D-28213 Bremen

http://www.thet= aphi.de

eMail: uwe@theta= phi.de

=A0

From:= Shai Erera [mailto:serera@= gmail.com]
Sent: Sunday, February 28, 2010 1:38 PM


To: = java-dev@lucene.apache.org
Subject: Re: SegmentInfos extends Vector

=A0

I would rather avoid implementing List .. we should implement Iterable for sure, bu= t I'd like to keep the API open either iterating in-order or getting a pa= rticular SegmentInfo. Another thing, I haven't seen anywhere that remove is call= ed. 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 cla= ss? If not, then we can deprecate it and introduce a new one ...

Shai

On Sun, Feb 28, 2010 at 2:25 PM, Uwe Schindler <uwe@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 U= OE. Just keep get() and so on.

=A0

-----

Uwe Schindler

H.-H.-Meier-Allee 63, D-28213 Bremen

http://www.thet= aphi.de

eMail: uwe@theta= phi.de

=A0

From:= Shai Erera [mailto:serera@= gmail.com]
Sent: Sunday, February 28, 2010 1:20 PM

Subject:<= /b> Re: SegmentInfos extends Vector

=A0

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 ha= ve 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 experiment= al 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 LUCEN= E-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 <uwe@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. B= ut maybe we should simply change it, it=92s a package-private class, right?

=A0

But in general subclassing those implementations is not the best thing you can do. In general the clas= s should extend Object or something else and just have final field of type List<=85>. Exposing the whole API of List to the outside is bad.

=A0

+1 to refactor this class (and don=92t let it extend a Collections class).

=A0

-----

Uwe Schindler

H.-H.-Meier-Allee 63, D-28213 Bremen

http://www.thet= aphi.de

eMail: uwe@theta= phi.de

=A0

From:= Shai Erera [mailto:serera@= gmail.com]
Sent: Sunday, February 28, 2010 12:33 PM
To: = java-dev@lucene.apache.org
Subject: SegmentInfos extends Vector

=A0

Hi

What's the reason SegmentInfos extends Vector rather than say ArrayList= ? Do we need the synchronization around it which Vector provides?

Shai

=A0

=A0

=A0


--000e0ce005a47dfb2e0480a90569--