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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add concurrency to GetUSD #766
Conversation
core/tokenrate/tokenrate.go
Outdated
errs = append(errs, e) | ||
mu.Unlock() | ||
if len(errs) >= len(quotes) { | ||
finalErrCh <- fmt.Errorf("%w: %s", ErrNoAvailableQuoteQuery, errs) |
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.
only one message will be read from this channel, other routines with error will hang forever on write, since nobody will read msg. you need to create buffer for len(quotes) elements or create wait group to wait for all the tasks to finish
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.
Thanks for pointing out. I have made it simpler now. There was no need of second goroutine apparently, as it wasn't helping in anyway. Pls have another look and let me know.
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.
Also, there wasn't a need to listen for ctx.Done because of the way resty is designed. It returnes immediately after the parent context's deadline exceeds.
2fff3d7
to
ffc5cee
Compare
ffc5cee
to
640946c
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.
LGTM
The failed test case is not related to the PR changes, so mark it as pass. |
Manual system tests [success] with the following config
|
Thanks @peterlimg |
Changes
Fixes
GetUSD
to fetch data from all providers concurrently聽#754Tests
Tasks to complete before merging PR:
Associated PRs (Link as appropriate):