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

Propagate contexts down scaler call stacks #2202

Merged
merged 9 commits into from Oct 26, 2021
Merged

Conversation

arschles
Copy link
Contributor

@arschles arschles commented Oct 19, 2021

This PR begins to propagate context.Contexts down the call stacks of various scalers.

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)

  • Tests have been added

  • Update all http.NewRequest() calls to http.NewRequestWithContext()

  • Update calls to functions in the redis-go module to use contexts passed from callers, rather than just context.Background()

  • Update calls to PostgreSQL/MySQL/MSSQL Scaler (due to SQL's QueryRowContext) to contexts passed from callers

  • Same as ^^ but for MongoDB

  • A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified) (N/A)

  • A PR is opened to update the documentation on (repo) (if applicable) (N/A)

  • Changelog has been updated

Partially addresses #2190

Signed-off-by: Aaron <aaron@ecomaz.net>
@arschles arschles marked this pull request as draft October 19, 2021 19:00
Signed-off-by: Aaron <aaron@ecomaz.net>
@JorTurFer
Copy link
Member

Nice job! :)

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

Looking good! Thanks for doing this :)

Signed-off-by: Aaron <aaron@ecomaz.net>
Signed-off-by: Aaron <aaron@ecomaz.net>
…dding context to scaler Close interface

Signed-off-by: Aaron <aaron@ecomaz.net>
Signed-off-by: Aaron <aaron@ecomaz.net>
Signed-off-by: Aaron <aaron@ecomaz.net>
Signed-off-by: Aaron <aaron@ecomaz.net>
@arschles arschles marked this pull request as ready for review October 20, 2021 22:45
@arschles
Copy link
Contributor Author

@zroubalik @JorTurFer I believe this PR is ready for reviews, so I've moved it out of draft mode.

the changes are large when measured by number of files changed, but relatively small when measured by lines changed. that's because it's a largely mechanical change that ensures that context.Contexts are passed down the stack from the Scaler interface to the implementations of each scaler, and not manually created wherever possible. I tried to ensure that there is no net-new implementation code in this PR.

can you take a look when you get a chance?

@zroubalik zroubalik changed the title Beginning to propagate contexts down scaler call stacks Propagate contexts down scaler call stacks Oct 21, 2021
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

Looking good! Just minor nit in Changelog.

CHANGELOG.md Outdated Show resolved Hide resolved
@zroubalik zroubalik added this to the v2.5.0 milestone Oct 21, 2021
Signed-off-by: Aaron <aaron@ecomaz.net>
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

3 participants