Skip to content
This repository has been archived by the owner on Dec 9, 2024. It is now read-only.

New waitForDeployment check logic #215

Merged
merged 4 commits into from
Aug 19, 2020
Merged

New waitForDeployment check logic #215

merged 4 commits into from
Aug 19, 2020

Conversation

faxioman
Copy link
Contributor

@faxioman faxioman commented Aug 17, 2020

This pull request replace the current setInterval implementation with an exponential check (30 seconds of max check delay).
This type of check permits a longer wait period without flooding the kubernetes master.
A longer wait period is required for service with a considerable number of functions and with a runtime that needs a build step (.net, java, ...).

Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

Thanks! I just have one minor comment.

lib/helpers.js Outdated
Comment on lines 244 to 247
function setExponentialInterval(cb, initialDelay, maxDelay) {
let delay = initialDelay;
let timer;
const internalCb = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The naming cb internalCb is not that readable, I would use something more meaningful like targetFunction and timerWrapper.

Also, initialDelay doesn't seem like a very useful parameter now (it was more important before to calculate the total delay). Can we remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree: targetFunction and timerWrapper are a lot better than cb and internalCb 👍
About initialDelay (and maxDelay) parameters: the setExponentialInterval could be a multipurpose util function. This is the reason I added the two parameters. It's something like the original setInterval but with an exponential behaviour.
Anyway, If you prefer I can remove the two parameters ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, I didn't mean to delete the two parameters to control the interval, just the one for the initialDelay since it's less useful than maxDelay. In any case, having both doesn't hurt.

Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

Thanks for the changes

@andresmgot andresmgot merged commit b916fac into serverless:master Aug 19, 2020
@faxioman
Copy link
Contributor Author

Thank for merging @andresmgot!
I see that some tests were failed so this version is not pushed on npm registry.
Do you have any idea why these failures? I could try to fix those tests...

@andresmgot
Copy link
Contributor

That's a bit annoying, the tests are not that reliable. For example, in this branch, the test that failed was the one that deployes a scheduled function:

https://travis-ci.org/github/serverless/serverless-kubeless/builds/718959671#L772

But in master, a unit test failed:

https://travis-ci.org/github/serverless/serverless-kubeless/builds/719211821#L337

Unfortunately, I don't know what can be causing this flakiness (and I don't have that much time to allocate for this).

Will release the version manually, thanks for the heads up.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants