Skip to content
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

Pipeline Data Corruption #13286

Merged
merged 3 commits into from
May 22, 2024
Merged

Pipeline Data Corruption #13286

merged 3 commits into from
May 22, 2024

Conversation

EasterTheBunny
Copy link
Contributor

@EasterTheBunny EasterTheBunny commented May 21, 2024

The unit test TestDivide_Example was acting flakey in the CI pipeline which suggested a flaw in the divide and
multiply operations. When running the test, the expected result would be one of the input values or the division
result in failure cases. This implied that results were either received out of order or were being sorted incorrectly.

The pipeline runner does a final sort on the results, so that ruled out the received out of order possibility. On
inspection of the sorting index on each task, every index was the zero value. This resulted in occasional correct
and incorrect sorting, causing the test flake.

To correct the problem, the unset value for tasks was set to -1 and new checks were added to enforce 0-based indexing on explicitly set indexes.

jmank88
jmank88 previously approved these changes May 21, 2024
Copy link
Collaborator

@samsondav samsondav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add some checks to this to prevent being able to parse a pipeline with ambiguous ordering?

jmank88
jmank88 previously approved these changes May 22, 2024
samsondav
samsondav previously approved these changes May 22, 2024
Copy link
Collaborator

@samsondav samsondav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically a breaking change and should be marked as such, but since the behaviour if you mixed and matched unspecified+0 index was undefined anyway, it shouldn't affect anything

@EasterTheBunny EasterTheBunny added this pull request to the merge queue May 22, 2024
The unit test `TestDivide_Example` was acting flakey in the CI pipeline which suggested a flaw in the divide and
multiply operations. When running the test, the expected result would be one of the input values or the division
result in failure cases. This implied that results were either received out of order or were being sorted incorrectly.

The pipeline runner does a final sort on the results, so that ruled out the received out of order possibility. On
inspection of the sorting index on each task, every index was the zero value. This resulted in occasional correct
and incorrect sorting, causing the test flake.

To correct the problem, the test was updated such that the expected result has an index of `1`, leaving all
other tasks with a `0` index.
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 22, 2024
@EasterTheBunny EasterTheBunny dismissed stale reviews from samsondav and jmank88 via a8edcf2 May 22, 2024 17:31
@EasterTheBunny EasterTheBunny added this pull request to the merge queue May 22, 2024
Merged via the queue into develop with commit 6139126 May 22, 2024
106 checks passed
@EasterTheBunny EasterTheBunny deleted the BCF-3236-pipeline-task-order branch May 22, 2024 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants