[GH-613]:Fixed issue "API rate limit exceeded for user ID". #626
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* [MI-2481]:Fixed issue mattermost#681 on github * [MI-2481]:Fixed review fixes * [MI-2481]:Fixed review fixes * [MI-2481]:Fixed review fixes
|
Hello @Kshitij-Katiyar, Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #626 +/- ##
==========================================
- Coverage 15.63% 15.51% -0.13%
==========================================
Files 15 15
Lines 5518 5543 +25
==========================================
- Hits 863 860 -3
- Misses 4613 4641 +28
Partials 42 42
☔ View full report in Codecov by Sentry. |
hanzei
added
2: Dev Review
mickmister
suggested changes
Jan 3, 2023
There was a problem hiding this comment.
Great work on this @Kshitij-Katiyar 👍 I have just a few requests
* [MI-2547]:Fixed review fixes for Github issue 613 and PR 626 * [MI-2547]:Fixed review fixes * [MI-2547]:Fixed review fixes
mickmister
reviewed
Jan 6, 2023
mickmister
approved these changes
Jan 9, 2023
|
/update-branch |
|
We don't have permissions to update this PR, please contact the submitter to apply the update. |
|
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
hanzei
approved these changes
Feb 3, 2023
|
@Kshitij-Katiyar I have this deployed to take another look... I have the last commit deployed. I followed my steps from earlier this year. #626 (comment) Can you please see if you can reproduce this? |
|
@Kshitij-Katiyar 0/5 This might only repo with account data I have. Please advise. |
@DHaussermann I have fixed the above issue. Just needed a minor change. Please test this again |
DHaussermann
approved these changes
Jul 5, 2023
There was a problem hiding this comment.
Tested and passed.
This now passes functional testing.
- Ensured web app will periodically fetch updated counts when they have changed
- Refresh option works to get new counts right away
- Panic form comment above ☝️ is resolved
- No sign of rate limit issue is server logs anymore
LGTM!
|
@hanzei and @mickmister, I re-requested dev review but I'm now seeing there were not too many commits since last round. 1 dev re-review would probably have been plenty. Sorry I though Dev review approvals were much older as this task has been going for some time now. |
hanzei
approved these changes
Jul 5, 2023
server/plugin/graphql/lhs_request.go
Outdated
|
|
||
| if !flagAssignee { | ||
| for _, resp := range mainQuery.Assignee.Nodes { | ||
| resp := resp |
There was a problem hiding this comment.
There was a problem hiding this comment.
The resp := resp looks a little off to me. Alternatively we could do this:
for i := range mainQuery.Assignee.Nodes {
resp := mainQuery.Assignee.Nodes[i]There was a problem hiding this comment.
sure @mickmister
mickmister
reviewed
Jul 5, 2023
There was a problem hiding this comment.
Thanks for this @Kshitij-Katiyar! I gave this another review. Nothing blocking, just leaving some comments
| break | ||
| } | ||
|
|
||
| if err := c.executeQuery(ctx, &mainQuery, params); err != nil { |
There was a problem hiding this comment.
Are we ever fetching duplicate data here? e.g. If flagOpenPR is true, do we still request some PR data from GitHub?
There was a problem hiding this comment.
There could be a time when one of the 3 is nil but other entry still have some data to fetch. In that case we still need to call the API as we want to get the other part of the data and there is only 1 API call and a single query happening here. @mickmister
mickmister
approved these changes
Jul 10, 2023
mickmister
suggested changes
Jul 10, 2023
mickmister
approved these changes
Jul 12, 2023
|
@DHaussermann Can you please give this another pass when you have the chance? cc @mkdbns |
DHaussermann
approved these changes
Jul 18, 2023
DHaussermann
added
4: Reviews Complete
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
User's mattermost logs were full of API rate limit exceeded warn message due to many API calls simultaneously. Mainly due to the notifications count on the mattermost sidebar.
Please refer to this comment for further information:- API rate limit exceeded for user ID #613 (comment)
Issue #613