lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Shai Erera <ser...@gmail.com>
Subject Re: SegmentInfos extends Vector
Date Sun, 28 Feb 2010 15:41:49 GMT
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 <uwe@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: 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
>
> 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
>
> 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
>
> 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
>
> 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