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

Issue #28: Increase sleep timeout to prevent api rate limiting #29

Merged
merged 12 commits into from
Mar 5, 2020
Merged

Issue #28: Increase sleep timeout to prevent api rate limiting #29

merged 12 commits into from
Mar 5, 2020

Conversation

nesta219
Copy link

@nesta219 nesta219 commented Mar 2, 2020

Issue #, if available: #28

Description of changes:

  • Increased the sleep timeout value from 5 seconds to 30 seconds to avoid API throttling when checking the status of the CodeBuild job. In the issue, 60 seconds was thrown out as a value but I don't think it needs to be as long as that. 30 seconds should strike a good balance between not making too many API requests vs. not delaying the build too long.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@seebees
Copy link
Collaborator

seebees commented Mar 2, 2020

Let's take advantage to the fact that we pass this value.

  1. Update the comment here: https://github.com/aws-actions/aws-codebuild-run-build/pull/29/files#diff-8e5779a8483a5cf65daafa5bd5ee3714L67
    change it to wait seconds :)

  2. Wrap the call with a try/catch here: https://github.com/aws-actions/aws-codebuild-run-build/pull/29/files#diff-8e5779a8483a5cf65daafa5bd5ee3714L46
    And then if the error is
    [error]Rate exceeded
    then log the change and
    recurse with an extended the wait value.
    e.g.
    return waitForBuildEndTime({ ...sdk, wait: sdk.wait*2 }, { id, logs }, nextToken);

@nesta219
Copy link
Author

nesta219 commented Mar 2, 2020

@seebees Made a few updates. The basic idea is: make the API call, check if we errored, and if we did, check that it's a rate limiting error. If so, sleep for a bit and try again (after extending the sleep timer). If not, throw an error (because it's a genuine error from the AWS API and as such, the job should fail).

code-build.js Outdated Show resolved Hide resolved
code-build.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@seebees seebees left a comment

Choose a reason for hiding this comment

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

Need tests :)
But the basic idea looks good.
Thanks.

code-build.js Outdated Show resolved Hide resolved
@nesta219
Copy link
Author

nesta219 commented Mar 4, 2020

@seebees added tests to check for rate limiting retry logic and also to make sure it properly dies if it gets a different error from the AWS API

Copy link
Collaborator

@seebees seebees left a comment

Choose a reason for hiding this comment

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

This is looking great!

code-build.js Outdated Show resolved Hide resolved
code-build.js Outdated Show resolved Hide resolved
test/code-build-test.js Outdated Show resolved Hide resolved

const sdk = help(
() => {
//similar to the ret function in the helper, allows me to throw an error in a function or return a more standard reply
Copy link
Collaborator

Choose a reason for hiding this comment

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

Take a look at: https://sembr.org/
You might like it.
It makes the diff of comments much nicer
and easier to read in diff tools.

@@ -318,6 +318,104 @@ describe("waitForBuildEndTime", () => {
});
expect(test).to.equal(buildReplies.pop().builds[0]);
});

it("waits after being rate limited and tries again", async function() {
this.timeout(45000);
Copy link
Collaborator

Choose a reason for hiding this comment

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

see below, this can be dropped for both tests :)

];

const sdk = help(
() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm going to say leave it now,
but maybe this functionality should be pulled into the root help function?

code-build.js Outdated Show resolved Hide resolved
code-build.js Outdated Show resolved Hide resolved
code-build.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@seebees seebees left a comment

Choose a reason for hiding this comment

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

Looks good,
thanks!

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.

None yet

2 participants