search

piceaTech / node-gitlab-2-github

Migrate Issues, milestones etc from gitlab to github
MIT License
547 stars 134 forks source link

Pause for an extra 60 secs after abuse limit errors before retrying #109

Closed akaihola closed 2 years ago

akaihola commented 2 years ago

Fixes #28.

mattip commented 2 years ago

Cool. You may want to raise the 60 seconds to 15 minutes?

mattip commented 2 years ago

Also - the unconditional catches on line 308 and 329 in src/index.ts means that the script will continue on happily processing when throttling errors happen, no? Does there need to be a retry mechanism or does throttling automatically retry?

mahalel commented 2 years ago

Hey, I've just tested this and it does seem that the script will keep processing when throttling errors happen still. Unfortunately I don't really know TypeScript to suggest a fix.

akaihola commented 2 years ago

Cool. You may want to raise the 60 seconds to 15 minutes?

The problem with that is that if 15 minutes isn't sufficient, it will sleep for another 15 minutes which probably is almost 15 minutes too much. I did try and think of a way to do a 15-minute sleep followed by as many 1-minute sleeps as necessary, but couldn't think of a simple way to achieve that.

According to our tests, the abuse limit lock gets released in 20 to 23 minutes.

the script will continue on happily processing when throttling errors happen

With this patch, the script should keep retrying every 65 seconds when it receives either an abuse limit error or a rate limit error response. When the request eventually gets through, it will continue processing the next request as if nothing unusual happened.

This is intentional and the point of the patch – we want to leave the script running and have it complete with exactly the same end result as would have achieved if no abuse/throttling errors were received.

I feel I don't entirely understand your concerns @mattip & @mahalel, could you clarify?

mattip commented 2 years ago

My concerns were based on misunderstanding how the throttling works. Thanks for the explanation. So a retry does not reset the lock timeout? That is good to know.

larstiq commented 2 years ago

Perhaps the behavior would be more clearly telegraphed if the PR was titled Pause for an extra 60 secs after abuse limit errors before retrying, rather than Stop which sounds final (to me).