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 <ser...@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 <rcm...@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 <ser...@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 <rcm...@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 <ser...@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-unsubscr...@lucene.apache.org >> >> For additional commands, e-mail: dev-h...@lucene.apache.org >> >> >> > >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org >> For additional commands, e-mail: dev-h...@lucene.apache.org >> >> >
