-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Do not use WaitGroup context for StepEvaluator #5425
Conversation
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.
LGTM
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.
Is there a way we can test this? Or is it covered by existing tests?
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.
Nice find. :).
Do you think worth adding test for that cancel()
?
Yeah we can add a tests, I wasn't sure if it was worth the investment because it's a bit tricky but doable. If we think we want it strongly I'll add it. |
* Correctly sets hash value for headblock iterator (#5423) * Do not use WaitGroup context for StepEvaluator (#5425) Co-authored-by: Cyril Tovena <[email protected]>
What this PR does / why we need it:
When we introduce parallelism in binops, we use a waitGroup but once we have waited it cancel the context and so when iterating it would actually stop right away.
This handle the cancellation in case of mutual failure manually and use the wait group without the context.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Checklist
CHANGELOG.md
about the changes.