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

Concurrently like all repos at once with goroutines #12

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JacobGo
Copy link

@JacobGo JacobGo commented Jan 4, 2021

I found that I can improve performance on potentially huge projects by liking all repos at the same time instead of sequentially. Used a separate goroutine for each repo, synced by a WaitGroup.

Some additional thoughts:

  • Consider replacing each call to client.Activity.IsStarred() with a single initial call to client.Activity.ListStarred().
  • Not sure how GitHub API might behave with hundreds of simultaneous requests.

I am quite new to Go and this is my first open source contribution in the language so please let me know if I missed something. Thanks!

@psampaz
Copy link
Owner

psampaz commented Mar 1, 2021

Hi @JacobGo, sorry for my late reply and thank you for this PR.

I do not know if I like the idea of sending requests concurrently, since according to Github recommendations:

Make requests for a single user or client ID serially. Do not make requests for a single user or client ID concurrently.

Source https://docs.github.com/en/rest/guides/best-practices-for-integrators#dealing-with-abuse-rate-limits

If you can reference an official Github source with concurrency limits, I might consider moving with this forward.

But, since you recently started with Go, my feedback for the PR is that is it fine. The only thing that I would add is some limit in concurrency using the idea described here http://jmoiron.net/blog/limiting-concurrency-in-go/.

thank you

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.

2 participants