Guard Get operations against Engine resets #125646
Conversation
Relates elastic#124635 Closes ES-11324
fcofdez
added
>non-issue
:Distributed Indexing/CRUD
|
Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing) |
| // TODO ES-10826: Acquire ref to engine and retry if it's immutable again? | ||
| l.onResponse(null); | ||
| })); | ||
| indexEventListener.beforeIndexShardMutableOperation(this, listener.delegateFailure((l, unused) -> l.onResponse(null))); |
There was a problem hiding this comment.
I think that we shouldn't provide the Engine through this listener. It kind of breaks the withEngine API that expects that the Engine won't escape the lambda.
There was a problem hiding this comment.
The unhollow/hollow timing should protect the engine from going hollow again.
There was a problem hiding this comment.
Yes, I think we can remove this TODO.
| // TODO ES-10826: Acquire ref to engine and retry if it's immutable again? | ||
| l.onResponse(null); | ||
| })); | ||
| indexEventListener.beforeIndexShardMutableOperation(this, listener.delegateFailure((l, unused) -> l.onResponse(null))); |
There was a problem hiding this comment.
Yes, I think we can remove this TODO.
|
Interesting test failure on |
|
Good point @arteam. I think that I'll remove the test |
|
There is special handling for retrying AlreadyClosedException in the serverless test stress to adjust. |
elasticsearchmachine
added
the
serverless-linked
|
Thanks both for noticing, I've opened the serverless counterpart of this. |
| if (cause instanceof ShardNotFoundException | ||
| || cause instanceof IndexNotFoundException | ||
| || cause instanceof IllegalIndexShardStateException | ||
| || cause instanceof AlreadyClosedException) { |
There was a problem hiding this comment.
This AlreadyClosedException is thrown when the engine is closed due to relocation (not due to reset)
| Engine engine = indexShard.getEngineOrNull(); | ||
| if (engine == null) { | ||
| throw new AlreadyClosedException("engine closed"); | ||
| // Allows to keep the same engine instance for getFromTranslog and getLastUnsafeSegmentGenerationForGets |
There was a problem hiding this comment.
Not strictly required, but I think it makes sense to ensure it uses the same engine instance.
|
Thanks all! |
Relates #124635
Closes ES-11324