-
Notifications
You must be signed in to change notification settings - Fork 35
#181 issue #182
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
#181 issue #182
Conversation
Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application. When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated. If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public. |
@rlagyu0, could you please sign the OCA? Thank you. |
collector/collector.go
Outdated
errmutex.Unlock() | ||
|
||
if err1 != nil { | ||
err = err1 |
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 think you're on the right track here, but this moves shared memory access outside the mutex boundary. I think we need to refactor this method and goroutine closure to properly synchronize the errors and provide an accurate count of errors in the defer
method. Likely this should be handled with an error channel, so we can collect all errors that occurred in the goroutine and remove shared memory access entirely.
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.
What happens if the content of err is changed by another goroutine after this method? It seems like this could also be a partial error. As you mentioned, I’ve placed logic in defer to handle both scrape errors and single errors. What do you think of this approach?
Please take a look at the next commit.
commit 6b775c6
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.
This is looking better, definitely an improvement over the original code. Thanks for incorporating an error channel!
I'd like to take this commit once we get the CLA issue sorted, and we can further optimize/clean-up this concurrent section from there.
|
Hi @rlagyu0, your OCA approved, but since your email 01086055223@hanmail.net associated with https://github.com/kimgy0 account but not with https://github.com/rlagyu0 the check failed. Please resign commit using kimgy0 account or associate the email with rlagyu0 account. |
I have unlinked the email 01086055223@hanmail.net from the previous account rlagy0. It is now linked to my current account, rlagyu0, and is being used as the primary email. |
See your PR signed by two your accounts
The OCA check passed only for [01086055223@hanmail.net](@kimgy0) but not for rlagyu0. |
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.
approving in order to run oca check again, with override.
please continue to work with anders on the implementation
issue 181