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.
From: Shai Erera
Sent: Sunday, February 28, 2010 2:22 PM
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.
On Sun, Feb 28, 2010 at 3:14 PM, Uwe Schindler <firstname.lastname@example.org> 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.
Sent: Sunday, February 28, 2010 2:04 PM
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.
Sun, Feb 28, 2010 at 2:49 PM, Uwe Schindler <email@example.com> wrote:
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
Also other classes got this interface in Lucene. Also adding j.io.Closeable everywhere
was a good idea.
From: Shai Erera
Sent: Sunday, February 28, 2010 1:38 PM
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 ...
Sun, Feb 28, 2010 at 2:25 PM, Uwe Schindler <firstname.lastname@example.org> 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
Sent: Sunday, February 28, 2010 1:20 PM
SegmentInfos extends Vector
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
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
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
Sun, Feb 28, 2010 at 1:37 PM, Uwe Schindler <email@example.com> 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).
What's the reason SegmentInfos extends Vector rather than say ArrayList? Do we
need the synchronization around it which Vector provides?