-
Notifications
You must be signed in to change notification settings - Fork 1.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
Use errGroup in handleValuePostings #5138
Conversation
cade3e4
to
b45ae33
Compare
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.
Reviewed 2 of 2 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @manishrjain)
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.
I assume this is just a refactor, there is no additional functionality? If so,
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @manishrjain)
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jarifibrahim and @manishrjain)
worker/task.go, line 495 at r1 (raw file):
s := start e := end
I don't think you need to do this. I am assuming this is to copy the values and prevent them from changing. But these values will be copied to the arguments because they are just numbers if you just call return calculate(start, end)
instead
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @manishrjain and @martinmr)
worker/task.go, line 495 at r1 (raw file):
Previously, martinmr (Martin Martinez Rivera) wrote…
s := start e := end
I don't think you need to do this. I am assuming this is to copy the values and prevent them from changing. But these values will be copied to the arguments because they are just numbers if you just call
return calculate(start, end)
instead
Oh, right. They're being initialized inside the for loop. Nice catch @martinmr, Thank you!
This PR improves the handling of goroutines in handleValuePostings function. The function starts a bunch of goroutines but doesn't wait for them to complete. It returns when the first goroutine returns an error. This PR fixes that issue.
Description.
This PR improves the handling of goroutines in
handleValuePostings
function. The function starts a bunch of goroutines but doesn't wait for them to complete. This PR fixes that isssue.GitHub Issue or Jira number.
Partially fixes #3873
See also #3873 (comment)
Affected releases.
v20.03
Changelog tags.
N/A
Please indicate if this is a breaking change.
No
Please indicate if this is an enterprise feature.
No
Please indicate if documentation needs to be updated.
No
Please indicate if end to end testing is needed.
No
This change is