Ensure close is called under lock in the case of an engine failure#5800
Ensure close is called under lock in the case of an engine failure#5800s1monw merged 1 commit intoelastic:masterfrom
Conversation
There was a problem hiding this comment.
Typo... should be assertLockIsHel_d_
|
I like it! it makes things cleaner. Left some comments.. |
There was a problem hiding this comment.
should we just catch Throwable here to be safe?
There was a problem hiding this comment.
also applies to other places in the code below, if we decide to do it.
There was a problem hiding this comment.
well this is what the code used to do. I think we are find here as it is to be honest....
There was a problem hiding this comment.
This is an excess to the indexWriter without a lock. I think this can lead to an NPE if the shard is closed. I realize it's not part of the change, but I think we should deal with it.
There was a problem hiding this comment.
I will use a local version of the indexwriter here...
|
@bleskes thanks for the review - I pushed another commit |
There was a problem hiding this comment.
I think writer can be null if optimizeMutex is true when the method begins. It seems we have this recurrent pattern of calling ensureOpen and than getting the indexWriter to do something. Perhaps we can change ensureOpen to either throw an exception or to return a non-null writer (guaranteed) . This this can become ensureOpen().waitForMerges()
|
thx. Simon. Looking good. Left one last comment. I'm +1 on this otherwise. |
|
I fixed your last suggestion! thanks for all the reveiws @bleskes I think it's ready, if you don't object I'd like to rebase and push it. |
|
thx. ++1 :) |
Until today we did close the engine without aqcuireing the write lock since most calls were still holding a read lock. This commit removes the code that holds on to the readlock when failing the engine which means we can simply call #close()
Until today we did close the engine without aqcuireing the write lock
since most calls were still holding a read lock. This commit removes
the code that holds on to the readlock when failing the engine which
means we can simply call #close()