-
Notifications
You must be signed in to change notification settings - Fork 25.6k
[ML] Clean up inference indices on failed endpoint creation #136577
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ML] Clean up inference indices on failed endpoint creation #136577
Conversation
Prior to this change, if only one of the .inference or .secrets-inference indices was updated when creating an inference endpoint, the endpoint creation would fail, but the successfully written doc was not removed, leading to inconsistent document counts between the two indices. This commit removes any documents that were written in the case that a partial failure occurred, but does not change the behaviour in the case where no updates to the indices were made. - Invoke a cleanup listener if a partial failure occurred when storing inference endpoint information in the .inference and .secrets-inference indices - Refactor ModelRegistry to use BulkRequestBuilder.add(IndexRequestBuilder) instead of the deprecated BulkRequestBuilder.add(IndexRequest) - Include cause when logging bulk failure during inference endpoint creation - Add integration tests for the new behaviour Closes elastic#123726
Pinging @elastic/ml-core (Team:ML) |
Hi @DonalEvans, I've created a changelog YAML for you. |
} | ||
|
||
logBulkFailures(model.getConfigurations().getInferenceEntityId(), bulkItemResponses); | ||
boolean anySuccess = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: It might be slightly cleaner to move this boolean above the cleanupListener
. Then in cleanupListener
we can have a check for anySuccess
, if there was a success, then do the delete.
Then we don't need to track listenerToInvoke
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, done
…136577) Prior to this change, if only one of the .inference or .secrets-inference indices was updated when creating an inference endpoint, the endpoint creation would fail, but the successfully written doc was not removed, leading to inconsistent document counts between the two indices. This commit removes any documents that were written in the case that a partial failure occurred, but does not change the behaviour in the case where no updates to the indices were made. - Invoke a cleanup listener if a partial failure occurred when storing inference endpoint information in the .inference and .secrets-inference indices - Refactor ModelRegistry to use BulkRequestBuilder.add(IndexRequestBuilder) instead of the deprecated BulkRequestBuilder.add(IndexRequest) - Include cause when logging bulk failure during inference endpoint creation - Add integration tests for the new behaviour - Update docs/changelog/136577.yaml Closes elastic#123726 (cherry picked from commit 9abc0bd)
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
…136577) Prior to this change, if only one of the .inference or .secrets-inference indices was updated when creating an inference endpoint, the endpoint creation would fail, but the successfully written doc was not removed, leading to inconsistent document counts between the two indices. This commit removes any documents that were written in the case that a partial failure occurred, but does not change the behaviour in the case where no updates to the indices were made. - Invoke a cleanup listener if a partial failure occurred when storing inference endpoint information in the .inference and .secrets-inference indices - Refactor ModelRegistry to use BulkRequestBuilder.add(IndexRequestBuilder) instead of the deprecated BulkRequestBuilder.add(IndexRequest) - Include cause when logging bulk failure during inference endpoint creation - Add integration tests for the new behaviour - Update docs/changelog/136577.yaml Closes elastic#123726 (cherry picked from commit 9abc0bd)
…136577) Prior to this change, if only one of the .inference or .secrets-inference indices was updated when creating an inference endpoint, the endpoint creation would fail, but the successfully written doc was not removed, leading to inconsistent document counts between the two indices. This commit removes any documents that were written in the case that a partial failure occurred, but does not change the behaviour in the case where no updates to the indices were made. - Invoke a cleanup listener if a partial failure occurred when storing inference endpoint information in the .inference and .secrets-inference indices - Refactor ModelRegistry to use BulkRequestBuilder.add(IndexRequestBuilder) instead of the deprecated BulkRequestBuilder.add(IndexRequest) - Include cause when logging bulk failure during inference endpoint creation - Add integration tests for the new behaviour - Update docs/changelog/136577.yaml Closes elastic#123726
Prior to this change, if only one of the .inference or .secrets-inference indices was updated when creating an inference endpoint, the endpoint creation would fail, but the successfully written doc was not removed, leading to inconsistent document counts between the two indices.
This commit removes any documents that were written in the case that a partial failure occurred, but does not change the behaviour in the case where no updates to the indices were made.
Closes #123726