Fix IndexWriter.close() to not commit or wait for pending merges [LUCENE-4246]

[asfimport]

---

Migrated from LUCENE-4246 by Robert Muir (@rmuir), resolved Apr 08 2014
Attachments: LUCENE-4246.patch (versions: 2)
Linked issues:

• If IndexWriter is interrupted on close and is using a channel (mmap/nio), it can throw a ClosedByInterruptException and prevent you from opening a new IndexWriter in the same proceses if you are using Native locks. [LUCENE-4638] #5703
• SOLR-6002

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

Moving my comment from #5315:

Why is that different from OutputStream.close() calling flush()? Would you like to call flush() yourself everytime before you close()?

I don't think there's anything we should 'fix' ...

Perhaps, if you want to simplify, close() should just commit and NEVER wait for merges. If someone wants that, he can call commit() + waitForMerges().

[asfimport]

Uwe Schindler (@uschindler) (migrated from JIRA)

Perhaps, if you want to simplify, close() should just commit and NEVER wait for merges. If someone wants that, he can call commit() + waitForMerges().

Yes!

I just repeat from the other issue: As CMS is involved, e.g. CMS.close() is interruptible, as it calls join() on all spawned merge threads after informing them to stop working. But as they are separate threads, there is some wait operation involved. We can simplify here, but we should still make IndexWriter.close() not interruptible.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

Because is transactional. Outputstream is not

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

Still, I don't want to call commit(), then close() every time I want to close. And I think it will confuse users, i.e. we have close(), commit() and rollback(). We know what commit() and rollback() do, but what does close() without commit() does? Is it like rollback()? If so, should we remove rollback()?

I think that close() doing commit() is fine. Even if it is transactional. Someone can always call rollback() if he wants to close w/o commit. That's the purpose of rollback().

[asfimport]

Michael McCandless (@mikemccand) (migrated from JIRA)

Perhaps, if you want to simplify, close() should just commit and NEVER wait for merges. If someone wants that, he can call commit() + waitForMerges().

I think you mean waitForMerges(), then commit()? Else the merges aren't committed...

I think this idea (close simply closes) makes sense! But:

Would close() throw an exception if there were pending (uncommitted, un-rolled-back) changes? Ie to avoid the obvious trap of users on upgrading assuming close still implicitly calls commit else they silently lose their changes.

Also if merges are still running I think close() should throw an exception, else CMS will be silently starved (never able to complete a large merge) for apps that open IW, add/update some docs, commit, close. We'd need to open up a "stopAllMerges" call.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

Close should alias to rollback. Otherwise its broken. We
should fix this before we have to support backwards... Committing shit on close is a massive bug, no other transactional apis act this way.

If you are against this, fine, another option is to remove the transactional API.

But the current situation is broken beyond words.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

I don't think we need to worry too much about apps that close indexwriter each time they add a doc being starved. Such apps are likely optimizing each time too

[asfimport]

Uwe Schindler (@uschindler) (migrated from JIRA)

I am fine with either proposal, but this has nothing to do with the interruptible bug, because close() has to still "wait" for CMS to shutdown (the mergeScheduler.close() call would keep alive, and that one is interruptible at the moment). My patch in the other issue ensures that the close of the scheduler and shutting down all its threads works, although some threads were interrupted by Jetty or any other automatic shutdown. Thats all this issue was opened, this here is not related at all.

By this issue we just have less interruptible calls on close, but that can be done after discussion, so I want to fix issue #5315.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

I think its related: I think close() is too complicated today.

In my opinion close() should just close resources: we should try to minimize what it does so it can easily be
used in e.g. finally block.

As far as commit() goes, I think you should always be explicit about that and not mix it up with close().

As far as pending merges, if we want to have an option to do that, I would prefer it not be the default
Closeable behavior of close(), instead something separate like shutdown().

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

By this issue we just have less interruptible calls on close, but that can be done after discussion, so I want to fix issue LUCENE-4245.

I opened a separate issue because I think its related but separate. I think it would be good to fix #5315 first as its easier, I just want an issue open for the bigger issue of what close() does.

[asfimport]

Uwe Schindler (@uschindler) (migrated from JIRA)

OK. That's all I wanted to make the test failures go away!

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

Would close() throw an exception if there were pending (uncommitted, un-rolled-back) changes? Ie to avoid the obvious trap of users on upgrading assuming close still implicitly calls commit else they silently lose their changes.

I'm torn here mostly because rollback() closes the IndexWriter. if it didn't do this then the path to me seems clear.
But since it does, its confusing.

E.g., just looking at the JDBC Connection api as a parallel, its a little vague but I think some impls actually throw Exception here? officially its of course...

"It is strongly recommended that an application explicitly commits or rolls back an active transaction prior to calling the close method. If the close method is called and there is an active transaction, the results are implementation-defined."

Seeing that we used to commit() on close(), I think its probably best that we would throw an Exception (if possible, i have no idea at a glance how). Otherwise the behavior change would be a trap... this would make it a lot easier.

We already do this if you entered the first phase of a two-phase commit.

But I really don't think we should do anything with pending merges: like i said we could have a separate 'graceful slow shutdown' for that that waits for merges: its unrelated to transactional semantics.

Is it like rollback()? If so, should we remove rollback()?

I think we should leave ourselves open to the possibility that one day, maybe rollback() can be done without closing.

[asfimport]

Shai Erera (@shaie) (migrated from JIRA)

I still don't understand why change behavior that is out there for ages. Everyone is used to calling close() and that commits things. I agree with you that if from the beginning people would need to call commit() before close(), then that's better. But I don't think there is a good justification to change current behavior.

Now you're talking about detecting whether there were any changes or not – that doesn't simplify close() logic IMO.

In short, I don't feel that changing current behavior is going to do any good, on the contrary, it would confuse people.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

I still don't understand why change behavior that is out there for ages. Everyone is used to calling close() and that commits things.

I'm not used to it: its awkward for a transactional API.

I agree with you that if from the beginning people would need to call commit() before close(), then that's better.

Then we should fix it! Just because "well its already this broken way", is no reason at all to not change things.
We should fix things so they work in a sane way.

Previous behavior doesn't matter at all here.

[asfimport]

Yonik Seeley (@yonik) (migrated from JIRA)

But I don't think there is a good justification to change current behavior.

+1

We have a rollback call already. Changing the semantics of close() does not simplify anything (i.e. it doesn't make the issue that spawned this one any easier).