-
Notifications
You must be signed in to change notification settings - Fork 1
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
Caching and UPDATED-sorting for the Github Data Sync Algorithm #94
Caching and UPDATED-sorting for the Github Data Sync Algorithm #94
Conversation
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.
Code looks good, except idk what happens with dismissed reviews. Only thing that is missing is the fix in the story so the check runs through
private int comments; | ||
private PullRequestReviewDTO[] changesRequested; | ||
private PullRequestReviewDTO[] approvals; | ||
private PullRequestReviewDTO[] comments; |
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 with the dismissed reviews?
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.
Interesting point! They were counted as comments for now. This should be fixed in: #97.
In the future we might want to do some sort of state replacement similar to the original Python script.
changesApproved.get(), comments); | ||
changesRequestedSet.toArray(new PullRequestReviewDTO[changesRequestedSet.size()]), | ||
approvedSet.toArray(new PullRequestReviewDTO[approvedSet.size()]), | ||
commentSet.toArray(new PullRequestReviewDTO[commentSet.size()])); |
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 everything is fine for now, in the future we might want store the score for each activity also in the database and then we can just calculate rank and aggregated score more easily.
For example, a pull request review comes in via webhooks, we estimate the effort it took to do this review and store it in the database. I think as it is now the score is not stable since the PR might get more changes and reviews get dismissed.
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.
Yes, this is very true.
We for sure have to improve a few elements for a correct and performant long-running execution.
Motivation
The current data fetching algorithm didn't collect all the required data and had massive performance issues. This PR aims to fix both of these issues. With this PR, the table should be on-par with the data from the original Python script.
Description
This PR includes multiple improvements:
GET /leaderboard
now filters the timeframe for the data before calculating the responsePerformance Test
develop
-branchIn my test runs the new fetching process now takes about 3 minutes (53 PRs), in comparison to the original 1 minute (8 PRs) and 5 minutes without caching (53 PRs).
Screenshots (if applicable)
Checklist
General
Server (if applicable)