lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Uwe Schindler" <...@thetaphi.de>
Subject RE: SegmentInfos extends Vector
Date Sun, 28 Feb 2010 14:04:36 GMT
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/> http://www.thetaphi.de

eMail: uwe@thetaphi.de

 

From: Shai Erera [mailto:serera@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 <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. 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 <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, 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 <uwe@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 <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 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 <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 UOE. Just keep get()
and so on.

 

-----

Uwe Schindler

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

http://www.thetaphi.de <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 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 <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. 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 <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

 

 

 

 


Mime
View raw message