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

functions: fix route timeout #349

Merged
merged 4 commits into from
Nov 28, 2016
Merged

functions: fix route timeout #349

merged 4 commits into from
Nov 28, 2016

Conversation

ucirello
Copy link
Contributor

Fix #339

@ucirello ucirello changed the title functions: fix route timeout [WIP] functions: fix route timeout Nov 28, 2016
@ucirello ucirello changed the title [WIP] functions: fix route timeout functions: fix route timeout Nov 28, 2016
@denismakogon
Copy link
Contributor

So, here the thing, we need to have persisted models definitions out of code in order to have good enough user experience during upgrades from version X to version Y.

Another question, if i would create a route before this patch and then will upgrade functions API up to this patch how this code will handle cases where model wouldn't have Timeout attribute.

Along with that, this code doesn't actually validate if Timeout fits integer range.

Mentions: https://open-iron.slack.com/archives/functions/p1480362602001126

@ucirello ucirello changed the title functions: fix route timeout [WIP] functions: fix route timeout Nov 28, 2016
@ucirello
Copy link
Contributor Author

ucirello commented Nov 28, 2016

So, here the thing, we need to have persisted models definitions out of code in order to have good enough user experience during upgrades from version X to version Y.
Another question, if i would create a route before this patch and then will upgrade functions API up to this patch how this code will handle cases where model wouldn't have Timeout attribute.

Ideally, we shall have stable model definition by v1, and from there backward compatible changes only. This hasn't been discussed yet. We are still in the Alpha cycle where we all expect things to break often.

Currently, routes whose configuration has Timeout zero, will no longer have any timeout. Coding something around expected timeouts is a big mistake. Thus, it should impact no one doing it right - and it will create a minor nuisance for those using it.

Along with that, this code doesn't actually validate if Timeout fits integer range.

Indeed it doesn't. But also this PR is WIP. I shall align integer sizes once I regenerate client libraries.

@ucirello ucirello changed the title [WIP] functions: fix route timeout functions: fix route timeout Nov 28, 2016
cli.DurationFlag{
Name: "timeout",
Usage: "route timeout",
Value: 30 * time.Second,
Copy link
Contributor

Choose a reason for hiding this comment

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

60 or 30 seconds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it should actually be 30, because that what it was before. I am reducing the default in the other package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@pedronasser pedronasser merged commit a7a466f into master Nov 28, 2016
@pedronasser pedronasser deleted the fix-timeout branch November 28, 2016 22:53
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.

4 participants