[ML] Adding retries for starting model deployment #99673
Conversation
jonathan-buttner
added
>non-issue
:ml
|
Hi @jonathan-buttner, I've created a changelog YAML for you. |
| while (true) { | ||
| try { | ||
| return client.search(searchRequest).actionGet(); | ||
| } catch (SearchPhaseExecutionException e) { |
There was a problem hiding this comment.
It's highly likely that this will be wrapped in a RemoteTransportException, so catching it like this won't work.
I think you should catch Exception and then use something like:
if (ExceptionsHelper.unwrapCause(e) instanceof SearchPhaseExecutionException) {
// do the processing
} else {
throw e;
}
There was a problem hiding this comment.
I think ExceptionsHelper.unwrapCause(e) instanceof CircuitBreakingException should also be accepted as a reason to retry. I'm not clear if the CircuitBreakingException will always be wrapped in a SearchPhaseExecutionException. It seems safer to accept either as a reason to retry.
…tner/elasticsearch into ml-start-model-improvements
…-model-improvements
|
Pinging @elastic/ml-core (Team:ML) |
|
|
||
| if (failureCount >= retries) { | ||
| logger.warn(format("[%s] searching for model part failed %s times, returning failure", modelId, retries)); | ||
| throw e; |
There was a problem hiding this comment.
Should the retry logic in TrainedModelAssignmentNodeService now be removed? If the search has failed after N tries it trying to reload the entire model puts the cluster under more strain.
Instead of throwing e please wrap it in another exception with a sensible error message saying something along the lines of loading model [model_id] failed after [SEARCH_RETRY_LIMIT] retries. The deployment is now in a failed state, the error may be transient please stop the deployment and restart.
The error message ends up in the failed routing state here and will eventually make it's way back to the caller assuming the request did not time out.
There was a problem hiding this comment.
Ah good point 👍
There was a problem hiding this comment.
Dave and I talked about this offline and we don't need to adjust the retry logic in TrainedModelAssignmentNodeService because using ElasticsearchException cannot be unwrapped and will not trigger that retry logic.
jonathan-buttner
changed the title
droberts195
added
auto-backport-and-merge
💚 Backport successful
|
* Reducing chunk size and adding retries * Testing search part * Update docs/changelog/99673.yaml * Unwrapping exception * Updating changelog * Wrapping exception and tests * Adding comments
This PR reduces the chunk size when downloading a model to 1 MB. It also adds retry logic for deploying a model specifically for
SearchPhaseException. This exception is thrown when a CircuitBreakerError occurs during model deployment. By catching the exception and retrying in the restorer logic we can avoid reloading the entire model.Addresses part of: #99409
Testing
Retrying does occur:
I can still get CBE to occur though.