Skip to content
This repository was archived by the owner on Jan 19, 2024. It is now read-only.

Conversation

@ocombe
Copy link
Contributor

@ocombe ocombe commented Feb 1, 2019

More improvementd to avoid the API rate limit errors:

  • We only check the number of reviews on review events and then we cache this in Firebase so that we can update the Github status accordingly for the other type of events (avoid 0 to 2 unnecessary requests per event)
  • We only get the repo config once per event. Instead of calling Github to get the config in each function that needs it, we pass it around (avoid 1 to 2 unnecessary requests per event)

Total = 1 to 4 requests avoided per event (most of the time 4), which is ~50 to 75% less requests per event for the PRs.

@IgorMinar
Copy link
Contributor

I'm sad to see the conflict check go, but let's do it if we have to. Maybe @josephperrott's cherry-picking and rebasing bot would be a better place to check for that anyway?

Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a comment. One more q:

If the config changes, will we reapply the new rules to all issues and prs? If not then we should - but that should be a separate pr.

});
});

exports.deleteCachedConfigs = https.onRequest(async (request: Request, response: Response) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this fn here? It doesn't seem to be used by anything. It's also missing docs. Can you please explain and document? Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will delete the config cached in firebase. It's just in case, if we want to force the bot to do a hard refresh of the config from github (for example if the config changed, but the push event wasn't delivered, then the config in firebase is wrong and we need to force a refresh).
I can clean the firebase database manually, but with this function anyone without access to firebase can also force the refresh (could be useful for caretakers?).
I'll add a comment.

@ocombe
Copy link
Contributor Author

ocombe commented Feb 8, 2019

If the config changes, we can manually apply the new rules for triage. We could trigger those manual functions from within the config update function if you want. I'd rather keep them separate for now.

The status on the other end won't be updated until the next status event (or whatever it is that affect this status). This is because status are linked to commits, not PRs, it would take a LOT of request to update all of them, and I don't have a manual function for that yet.

Cache the config files for each repo in Firebase instead of requesting them from Github for each PR.
Whenever the config is needed, we get it from Firebase, or from Github if we don't have the config in cache (in which case we cache it at the same time).
Whenever there is a push commit to master, we check if the config file was updated, and if that's the case we update the cached config.
There is also a new get function that you can call to delete all of the cache. This can be used to force a refresh of the cache if the cached config is wrong (if we missed the push event for example).
@IgorMinar
Copy link
Contributor

IgorMinar commented Feb 8, 2019 via email

@ocombe
Copy link
Contributor Author

ocombe commented Feb 11, 2019

We could, but it's already updating status/triage if you add/remove one of the required labels (like target), I'm not sure if we need to add a specific option for that.

@ocombe ocombe merged commit 77ddd5b into angular:master Feb 11, 2019
@ocombe ocombe deleted the cache-more branch February 11, 2019 14:36
@IgorMinar
Copy link
Contributor

IgorMinar commented Feb 12, 2019 via email

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants