You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucene.apache.org by Shai Erera <se...@gmail.com> on 2014/08/13 15:00:29 UTC
Who should close MergePolicy
Hi
While working on LUCENE-5883, I noticed that IndexWriter calls
mergePolicy.close(). But since LUCENE-5711, where we no longer wire an MP
to a single IndexWriter instance, I don't think IW should call MP.close(),
e.g. in case the app shares this MP across several IWs?
But then, in the common case where an MP is used by a single IW instance,
having IW close it is convenient.
I looked at all MPs we have, none seem to implement close() in any special
way (empty impls, or delegating impls are the only ones I found). So
question is:
1) Is it a bug that IW calls mp.close()? I think so, but would like to
confirm with others.
2) If it is a bug, and we're going to say that you should close your MP
separately, I wonder why we need to have Closeable on MP at all. If someone
uses an MP which should be closed, he can just close it? This stems from
the fact that none of the existing MPs in trunk really implement close(),
yet we advertise that you should close MP since it implements Closeable.
Shai
Re: Who should close MergePolicy
Posted by Shai Erera <se...@gmail.com>.
Hmm, I think we have the same problem with MergeScheduler. It is no longer
wired to an IW instance, yet IW calls ms.close() when it's closed. This is
wrong ... maybe we should add ref-counting to MergeScheduler?
Shai
On Thu, Aug 14, 2014 at 8:38 AM, Shai Erera <se...@gmail.com> wrote:
> OK I removed it in the latest patch on LUCENE-5883.
>
>
> On Thu, Aug 14, 2014 at 8:17 AM, Robert Muir <rc...@gmail.com> wrote:
>
>> Yeah, I agree. We should only add such semantics if necessary. But
>> close() definitely seems wrong so lets just start with nuking that?
>>
>> On Thu, Aug 14, 2014 at 1:08 AM, Shai Erera <se...@gmail.com> wrote:
>> > OK I will remove it under LUCENE-5883.
>> >
>> > And I like the idea of adding ref-counting semantics, but we should do
>> it
>> > separately and only when there's a real reason too. Then, we can make
>> sure
>> > IW inc/decRef appropriately.
>> >
>> > Shai
>> >
>> >
>> > On Thu, Aug 14, 2014 at 8:05 AM, Robert Muir <rc...@gmail.com> wrote:
>> >>
>> >> Was there a reasoning for adding this method in the first place? What
>> was
>> >> it?
>> >>
>> >> Was it accidental?
>> >>
>> >> +1 for straightening this out: seems like if MP should support
>> >> multiple indexwriter instances then it should have a reference
>> >> counting API rather than abusing closeable semantics!
>> >>
>> >> On Wed, Aug 13, 2014 at 9:00 AM, Shai Erera <se...@gmail.com> wrote:
>> >> > Hi
>> >> >
>> >> > While working on LUCENE-5883, I noticed that IndexWriter calls
>> >> > mergePolicy.close(). But since LUCENE-5711, where we no longer wire
>> an
>> >> > MP to
>> >> > a single IndexWriter instance, I don't think IW should call
>> MP.close(),
>> >> > e.g.
>> >> > in case the app shares this MP across several IWs?
>> >> >
>> >> > But then, in the common case where an MP is used by a single IW
>> >> > instance,
>> >> > having IW close it is convenient.
>> >> >
>> >> > I looked at all MPs we have, none seem to implement close() in any
>> >> > special
>> >> > way (empty impls, or delegating impls are the only ones I found). So
>> >> > question is:
>> >> >
>> >> > 1) Is it a bug that IW calls mp.close()? I think so, but would like
>> to
>> >> > confirm with others.
>> >> >
>> >> > 2) If it is a bug, and we're going to say that you should close your
>> MP
>> >> > separately, I wonder why we need to have Closeable on MP at all. If
>> >> > someone
>> >> > uses an MP which should be closed, he can just close it? This stems
>> from
>> >> > the
>> >> > fact that none of the existing MPs in trunk really implement close(),
>> >> > yet we
>> >> > advertise that you should close MP since it implements Closeable.
>> >> >
>> >> > Shai
>> >>
>> >> ---------------------------------------------------------------------
>> >> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
>> >> For additional commands, e-mail: dev-help@lucene.apache.org
>> >>
>> >
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
>> For additional commands, e-mail: dev-help@lucene.apache.org
>>
>>
>
Re: Who should close MergePolicy
Posted by Shai Erera <se...@gmail.com>.
OK I removed it in the latest patch on LUCENE-5883.
On Thu, Aug 14, 2014 at 8:17 AM, Robert Muir <rc...@gmail.com> wrote:
> Yeah, I agree. We should only add such semantics if necessary. But
> close() definitely seems wrong so lets just start with nuking that?
>
> On Thu, Aug 14, 2014 at 1:08 AM, Shai Erera <se...@gmail.com> wrote:
> > OK I will remove it under LUCENE-5883.
> >
> > And I like the idea of adding ref-counting semantics, but we should do it
> > separately and only when there's a real reason too. Then, we can make
> sure
> > IW inc/decRef appropriately.
> >
> > Shai
> >
> >
> > On Thu, Aug 14, 2014 at 8:05 AM, Robert Muir <rc...@gmail.com> wrote:
> >>
> >> Was there a reasoning for adding this method in the first place? What
> was
> >> it?
> >>
> >> Was it accidental?
> >>
> >> +1 for straightening this out: seems like if MP should support
> >> multiple indexwriter instances then it should have a reference
> >> counting API rather than abusing closeable semantics!
> >>
> >> On Wed, Aug 13, 2014 at 9:00 AM, Shai Erera <se...@gmail.com> wrote:
> >> > Hi
> >> >
> >> > While working on LUCENE-5883, I noticed that IndexWriter calls
> >> > mergePolicy.close(). But since LUCENE-5711, where we no longer wire an
> >> > MP to
> >> > a single IndexWriter instance, I don't think IW should call
> MP.close(),
> >> > e.g.
> >> > in case the app shares this MP across several IWs?
> >> >
> >> > But then, in the common case where an MP is used by a single IW
> >> > instance,
> >> > having IW close it is convenient.
> >> >
> >> > I looked at all MPs we have, none seem to implement close() in any
> >> > special
> >> > way (empty impls, or delegating impls are the only ones I found). So
> >> > question is:
> >> >
> >> > 1) Is it a bug that IW calls mp.close()? I think so, but would like to
> >> > confirm with others.
> >> >
> >> > 2) If it is a bug, and we're going to say that you should close your
> MP
> >> > separately, I wonder why we need to have Closeable on MP at all. If
> >> > someone
> >> > uses an MP which should be closed, he can just close it? This stems
> from
> >> > the
> >> > fact that none of the existing MPs in trunk really implement close(),
> >> > yet we
> >> > advertise that you should close MP since it implements Closeable.
> >> >
> >> > Shai
> >>
> >> ---------------------------------------------------------------------
> >> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
> >> For additional commands, e-mail: dev-help@lucene.apache.org
> >>
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: dev-help@lucene.apache.org
>
>
Re: Who should close MergePolicy
Posted by Robert Muir <rc...@gmail.com>.
Yeah, I agree. We should only add such semantics if necessary. But
close() definitely seems wrong so lets just start with nuking that?
On Thu, Aug 14, 2014 at 1:08 AM, Shai Erera <se...@gmail.com> wrote:
> OK I will remove it under LUCENE-5883.
>
> And I like the idea of adding ref-counting semantics, but we should do it
> separately and only when there's a real reason too. Then, we can make sure
> IW inc/decRef appropriately.
>
> Shai
>
>
> On Thu, Aug 14, 2014 at 8:05 AM, Robert Muir <rc...@gmail.com> wrote:
>>
>> Was there a reasoning for adding this method in the first place? What was
>> it?
>>
>> Was it accidental?
>>
>> +1 for straightening this out: seems like if MP should support
>> multiple indexwriter instances then it should have a reference
>> counting API rather than abusing closeable semantics!
>>
>> On Wed, Aug 13, 2014 at 9:00 AM, Shai Erera <se...@gmail.com> wrote:
>> > Hi
>> >
>> > While working on LUCENE-5883, I noticed that IndexWriter calls
>> > mergePolicy.close(). But since LUCENE-5711, where we no longer wire an
>> > MP to
>> > a single IndexWriter instance, I don't think IW should call MP.close(),
>> > e.g.
>> > in case the app shares this MP across several IWs?
>> >
>> > But then, in the common case where an MP is used by a single IW
>> > instance,
>> > having IW close it is convenient.
>> >
>> > I looked at all MPs we have, none seem to implement close() in any
>> > special
>> > way (empty impls, or delegating impls are the only ones I found). So
>> > question is:
>> >
>> > 1) Is it a bug that IW calls mp.close()? I think so, but would like to
>> > confirm with others.
>> >
>> > 2) If it is a bug, and we're going to say that you should close your MP
>> > separately, I wonder why we need to have Closeable on MP at all. If
>> > someone
>> > uses an MP which should be closed, he can just close it? This stems from
>> > the
>> > fact that none of the existing MPs in trunk really implement close(),
>> > yet we
>> > advertise that you should close MP since it implements Closeable.
>> >
>> > Shai
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
>> For additional commands, e-mail: dev-help@lucene.apache.org
>>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
Re: Who should close MergePolicy
Posted by Shai Erera <se...@gmail.com>.
OK I will remove it under LUCENE-5883.
And I like the idea of adding ref-counting semantics, but we should do it
separately and only when there's a real reason too. Then, we can make sure
IW inc/decRef appropriately.
Shai
On Thu, Aug 14, 2014 at 8:05 AM, Robert Muir <rc...@gmail.com> wrote:
> Was there a reasoning for adding this method in the first place? What was
> it?
>
> Was it accidental?
>
> +1 for straightening this out: seems like if MP should support
> multiple indexwriter instances then it should have a reference
> counting API rather than abusing closeable semantics!
>
> On Wed, Aug 13, 2014 at 9:00 AM, Shai Erera <se...@gmail.com> wrote:
> > Hi
> >
> > While working on LUCENE-5883, I noticed that IndexWriter calls
> > mergePolicy.close(). But since LUCENE-5711, where we no longer wire an
> MP to
> > a single IndexWriter instance, I don't think IW should call MP.close(),
> e.g.
> > in case the app shares this MP across several IWs?
> >
> > But then, in the common case where an MP is used by a single IW instance,
> > having IW close it is convenient.
> >
> > I looked at all MPs we have, none seem to implement close() in any
> special
> > way (empty impls, or delegating impls are the only ones I found). So
> > question is:
> >
> > 1) Is it a bug that IW calls mp.close()? I think so, but would like to
> > confirm with others.
> >
> > 2) If it is a bug, and we're going to say that you should close your MP
> > separately, I wonder why we need to have Closeable on MP at all. If
> someone
> > uses an MP which should be closed, he can just close it? This stems from
> the
> > fact that none of the existing MPs in trunk really implement close(),
> yet we
> > advertise that you should close MP since it implements Closeable.
> >
> > Shai
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: dev-help@lucene.apache.org
>
>
Re: Who should close MergePolicy
Posted by Robert Muir <rc...@gmail.com>.
Was there a reasoning for adding this method in the first place? What was it?
Was it accidental?
+1 for straightening this out: seems like if MP should support
multiple indexwriter instances then it should have a reference
counting API rather than abusing closeable semantics!
On Wed, Aug 13, 2014 at 9:00 AM, Shai Erera <se...@gmail.com> wrote:
> Hi
>
> While working on LUCENE-5883, I noticed that IndexWriter calls
> mergePolicy.close(). But since LUCENE-5711, where we no longer wire an MP to
> a single IndexWriter instance, I don't think IW should call MP.close(), e.g.
> in case the app shares this MP across several IWs?
>
> But then, in the common case where an MP is used by a single IW instance,
> having IW close it is convenient.
>
> I looked at all MPs we have, none seem to implement close() in any special
> way (empty impls, or delegating impls are the only ones I found). So
> question is:
>
> 1) Is it a bug that IW calls mp.close()? I think so, but would like to
> confirm with others.
>
> 2) If it is a bug, and we're going to say that you should close your MP
> separately, I wonder why we need to have Closeable on MP at all. If someone
> uses an MP which should be closed, he can just close it? This stems from the
> fact that none of the existing MPs in trunk really implement close(), yet we
> advertise that you should close MP since it implements Closeable.
>
> Shai
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
Re: Who should close MergePolicy
Posted by Michael McCandless <lu...@mikemccandless.com>.
+1 to just remove Closeable from MP!
Mike McCandless
http://blog.mikemccandless.com
On Wed, Aug 13, 2014 at 9:00 AM, Shai Erera <se...@gmail.com> wrote:
> Hi
>
> While working on LUCENE-5883, I noticed that IndexWriter calls
> mergePolicy.close(). But since LUCENE-5711, where we no longer wire an MP to
> a single IndexWriter instance, I don't think IW should call MP.close(), e.g.
> in case the app shares this MP across several IWs?
>
> But then, in the common case where an MP is used by a single IW instance,
> having IW close it is convenient.
>
> I looked at all MPs we have, none seem to implement close() in any special
> way (empty impls, or delegating impls are the only ones I found). So
> question is:
>
> 1) Is it a bug that IW calls mp.close()? I think so, but would like to
> confirm with others.
>
> 2) If it is a bug, and we're going to say that you should close your MP
> separately, I wonder why we need to have Closeable on MP at all. If someone
> uses an MP which should be closed, he can just close it? This stems from the
> fact that none of the existing MPs in trunk really implement close(), yet we
> advertise that you should close MP since it implements Closeable.
>
> Shai
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org