Refresh should not acquire readLock#48414
Conversation
|
Pinging @elastic/es-distributed (:Distributed/Engine) |
|
A known issue tracked at #46021. @elasticmachine run elasticsearch-ci/1 |
henningandersen
left a comment
There was a problem hiding this comment.
This looks good to me, but not sure that counts for this issue. I did try to find a hole in the plot (and were unable to), but the other reviewers have the necessary historic background to do a qualified review.
Maybe also add a test to demonstrate that we do not hold the lock?
+1. I added that test in 37e1558. |
ywelsch
left a comment
There was a problem hiding this comment.
LGTM. Can you hold off a bit (e.g. a week) to merge this into 7.5 branch (fine to merge to other branches). I would like to give this enough CI testing before it goes into our product.
|
@henningandersen @jpountz @ywelsch Thanks for reviewing. |
Today, we hold the engine readLock while refreshing. Although this choice simplifies the correctness reasoning, it can block IndexShard from closing if warming an external reader takes time. The current implementation of refresh does not need to hold readLock as ReferenceManager can handle errors correctly if the engine is closed in midway. This PR is a prerequisite that we need to solve #47186.
+1 |
Today, we hold the engine readLock while refreshing. Although this choice simplifies the correctness reasoning, it can block IndexShard from closing if warming an external reader takes time. The current implementation of refresh does not need to hold readLock as ReferenceManager can handle errors correctly if the engine is closed in midway. This PR is a prerequisite that we need to solve #47186.
Today, we hold the engine readLock while refreshing. Although this choice simplifies the correctness reasoning, it can block IndexShard from closing if warming an external reader takes time. The current implementation of refresh does not need to hold readLock as ReferenceManager can handle errors correctly if the engine is closed in midway.
This PR is a prerequisite that we need to solve #47186.
Relates #47186