[ML][Transform] reset failure count when a transform aggregation page is handled successfully #76355
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… is handled successfully
Collaborator
|
Pinging @elastic/ml-core (Team:ML) |
benwtrent
commented
Aug 11, 2021
Comment on lines
+150
to
+158
| if (bulkResponse.hasFailures() == false) { | ||
| // We don't know the of failures that have occurred (searching, processing, indexing, etc.), | ||
| // but if we search, process and bulk index then we have | ||
| // successfully processed an entire page of the transform and should reset the counter, even if we are in the middle | ||
| // of a checkpoint | ||
| context.resetReasonAndFailureCounter(); | ||
| nextPhase.onResponse(bulkResponse); | ||
| return; | ||
| } |
Member
Author
There was a problem hiding this comment.
OK, this diff is a pain. The only difference here is the resetReasonAndFailureCounter and the comments. I created a new method here so that this is testable.
benwtrent
commented
Aug 11, 2021
| this.auditor = transformServices.getAuditor(); | ||
| this.transformConfig = ExceptionsHelper.requireNonNull(transformConfig, "transformConfig"); | ||
| this.progress = progress != null ? progress : new TransformProgress(); | ||
| this.progress = transformProgress != null ? transformProgress : new TransformProgress(); |
Member
Author
There was a problem hiding this comment.
OK, this seemed like a bug, progress was assigning to itself, which was never set? We should set it to the passed value.
There was a problem hiding this comment.
That was broken in #75459, which is a 7.15 change, and explains why it hasn't caused a bug report from a user yet.
benwtrent
added
v7.14.1
auto-backport
benwtrent
deleted the
transforms/reset-failure-count-on-indexing-success
branch
Collaborator
💔 Backport failed
To backport manually run: |
benwtrent
added a commit
to benwtrent/elasticsearch
that referenced
this pull request
Aug 11, 2021
… is handled successfully (elastic#76355) Failure count should not only be reset at checkpoints. Checkpoints could have many pages of data. Consequently, we should reset the failure count once we handle a single composite aggregation page. This way, the transform won't mark itself as failed erroneously when it has actually succeeded searches + indexing results within the same checkpoint. closes elastic#76074
benwtrent
added a commit
to benwtrent/elasticsearch
that referenced
this pull request
Aug 11, 2021
… is handled successfully (elastic#76355) Failure count should not only be reset at checkpoints. Checkpoints could have many pages of data. Consequently, we should reset the failure count once we handle a single composite aggregation page. This way, the transform won't mark itself as failed erroneously when it has actually succeeded searches + indexing results within the same checkpoint. closes elastic#76074
elasticsearchmachine
pushed a commit
that referenced
this pull request
Aug 11, 2021
…n page is handled successfully (#76355) (#76365) * [ML][Transform] reset failure count when a transform aggregation page is handled successfully (#76355) Failure count should not only be reset at checkpoints. Checkpoints could have many pages of data. Consequently, we should reset the failure count once we handle a single composite aggregation page. This way, the transform won't mark itself as failed erroneously when it has actually succeeded searches + indexing results within the same checkpoint. closes #76074 * fixing compilation
elasticsearchmachine
pushed a commit
that referenced
this pull request
Aug 11, 2021
…on page is handled successfully (#76355) (#76366) * [ML][Transform] reset failure count when a transform aggregation page is handled successfully (#76355) Failure count should not only be reset at checkpoints. Checkpoints could have many pages of data. Consequently, we should reset the failure count once we handle a single composite aggregation page. This way, the transform won't mark itself as failed erroneously when it has actually succeeded searches + indexing results within the same checkpoint. closes #76074 * fixing tests * fixing tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Failure count should not only be reset at checkpoints. Checkpoints could have many pages of data. Consequently, we should reset the failure count once we handle a single composite aggregation page.
This way, the transform won't mark itself as failed erroneously when it has actually succeeded searches + indexing results within the same checkpoint.
closes #76074