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 16:53:28 GMT
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 <
lucene@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 <serera@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 <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
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: java-dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: java-dev-help@lucene.apache.org
>
>

Mime
View raw message