EQL: Fix early trimming of in-flight data#66493
Merged
costin merged 1 commit intoelastic:masterfrom Dec 17, 2020
Merged
Conversation
6fc9ba0 to
2e0d465
Compare
Rework trimToLast to take into account an ordinal for last trimming so instead of keeping the last entry in a stage, it keeps the last entry before the given ordinal. This takes care of the case where a dense stage that requires several passes does not discard valid data from a previous sparse stage that go beyond the current stage point.
2e0d465 to
5a63383
Compare
Collaborator
|
Pinging @elastic/es-ql (Team:QL) |
bpintea
approved these changes
Dec 17, 2020
| // and remember the last item from the first | ||
| // initialized stage to be used with until | ||
| // remember the last item found (will be ascending) | ||
| // to trim unneeded until that occur before it |
Contributor
There was a problem hiding this comment.
Suggested change
| // to trim unneeded until that occur before it | |
| // to trim the ones before it |
|
|
||
| /** | ||
| * Returns the latest element from the group that has its timestamp | ||
| * less than the given argument alongside its position in the list. |
Contributor
There was a problem hiding this comment.
Suggested change
| * less than the given argument alongside its position in the list. | |
| * less than the given argument's timestamp, alongside its position in the list. |
| /** | ||
| * Returns the latest element from the group that has its timestamp | ||
| * less than the given argument alongside its position in the list. | ||
| * Everything before the element it is removed. The element is kept. |
Contributor
There was a problem hiding this comment.
Suggested change
| * Everything before the element it is removed. The element is kept. | |
| * Everything before this element is removed. The element is kept. |
Member
Author
There was a problem hiding this comment.
Will pick them up in a separate PR to avoid another build.
| * and adjust insertion positions. | ||
| */ | ||
| void trim(boolean everything) { | ||
| void trim(Ordinal ordinal) { |
Contributor
There was a problem hiding this comment.
Wondering if @Nullable would be welcome here, as an indicator of null as a branching value?
Member
Author
There was a problem hiding this comment.
I'm not a fan of @Nullable since it doesn't enforce anything nor it is used consistently through-out the code.
matriv
approved these changes
Dec 17, 2020
Contributor
matriv
left a comment
There was a problem hiding this comment.
LGTM, Is there any integration test that can be used to validate this fix?
Member
Author
Yes, the correctness test with tail by default. Our smaller tests seems to trigger it but I haven't had time to double check if this is indeed the case. |
costin
added a commit
that referenced
this pull request
Dec 17, 2020
Rework trimToLast to take into account an ordinal for last trimming so instead of keeping the last entry in a stage, it keeps the last entry before the given ordinal. This takes care of the case where a dense stage that requires several passes does not discard valid data from a previous sparse stage that go beyond the current stage point. (cherry picked from commit 4f55749)
costin
added a commit
that referenced
this pull request
Dec 17, 2020
Rework trimToLast to take into account an ordinal for last trimming so instead of keeping the last entry in a stage, it keeps the last entry before the given ordinal. This takes care of the case where a dense stage that requires several passes does not discard valid data from a previous sparse stage that go beyond the current stage point. (cherry picked from commit 4f55749) (cherry picked from commit 6b61dfe)
costin
added a commit
that referenced
this pull request
Dec 17, 2020
Rework trimToLast to take into account an ordinal for last trimming so instead of keeping the last entry in a stage, it keeps the last entry before the given ordinal. This takes care of the case where a dense stage that requires several passes does not discard valid data from a previous sparse stage that go beyond the current stage point. (cherry picked from commit 4f55749) (cherry picked from commit 6b61dfe) (cherry picked from commit cece81b)
Member
Author
|
Backported to 7.x, 7.11 and 7.10 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 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.
Rework trimToLast to take into account an ordinal for last trimming so
instead of keeping the last entry in a stage, it keeps the last entry
before the given ordinal.
This takes care of the case where a dense stage that requires several
passes does not discard valid data from a previous sparse stage that go
beyond the current stage point.