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.

Sign up for GitHub

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

Jump to bottom

Allow setting MaxConcurrentReconciles for reconcilers #2272

Merged
merged 7 commits into from
Nov 23, 2021
Merged

Allow setting MaxConcurrentReconciles for reconcilers #2272

merged 7 commits into from
Nov 23, 2021

Conversation

zroubalik
Copy link
Member

@zroubalik zroubalik commented Nov 11, 2021 •
edited
Loading

Signed-off-by: Zbynek Roubalik zroubali@redhat.com

This should allow setting MaxConcurrentReconciles for controllers.

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Changelog has been updated

@zroubalik zroubalik marked this pull request as draft November 11, 2021 11:29
@zroubalik
Copy link
Member Author

/run-e2e

2 similar comments
@zroubalik
Copy link
Member Author

/run-e2e

@zroubalik
Copy link
Member Author

/run-e2e

@zroubalik zroubalik added this to the v2.5.0 milestone Nov 11, 2021
@zroubalik zroubalik changed the title allow setting MaxConcurrentReconciles for controllers Allow setting MaxConcurrentReconciles for controllers Nov 11, 2021
@zroubalik zroubalik marked this pull request as ready for review November 11, 2021 22:35
@zroubalik zroubalik changed the title Allow setting MaxConcurrentReconciles for controllers Allow setting MaxConcurrentReconciles for controllers Nov 11, 2021
@zroubalik
Copy link
Member Author

zroubalik commented Nov 11, 2021 •
edited
Loading

A question is what is the correct default value? ATM it is on 1. But I think we should increase it at least for ScaledObjectReconciler, what is the right value? 5? 10?

@ahmelsayed @arschles @JorTurFer WDYT?

I'll open docs update PR based on the default value decision

@zroubalik zroubalik changed the title Allow setting MaxConcurrentReconciles for controllers Allow setting MaxConcurrentReconciles for reconcilers Nov 11, 2021
@JorTurFer
Copy link
Member

A question is what is the correct default value? ATM it is on 1. But I think we should increase it at least for ScaledObjectReconciler, what is the right value? 5? 10?

@ahmelsayed @arschles @JorTurFer WDYT?

I'll open docs update PR based on the default value decision

Maybe we could I increase to 5 and check the performance during next release.
Apart from the docs, I think that we could expose the parameter in the helm chart to allow to modify it easily.
Wdty?

JorTurFer
JorTurFer approved these changes Nov 12, 2021
View reviewed changes
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

LGTM!

arschles
arschles suggested changes Nov 12, 2021
View reviewed changes
adapter/main.go Outdated Show resolved Hide resolved
controllers/keda/scaledobject_controller.go Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@ahmelsayed
Copy link
Contributor

A question is what is the correct default value? ATM it is on 1. But I think we should increase it at least for ScaledObjectReconciler, what is the right value? 5? 10?
@ahmelsayed @arschles @JorTurFer WDYT?
I'll open docs update PR based on the default value decision

Maybe we could I increase to 5 and check the performance during next release. Apart from the docs, I think that we could expose the parameter in the helm chart to allow to modify it easily. Wdty?

I don't have any strong opinions on a particular number. 5 sounds good to me.

@zroubalik
Copy link
Member Author

@arschles I've created a function for ENV parsing. PTAL

@zroubalik zroubalik mentioned this pull request Nov 23, 2021
Merged
1 task
@zroubalik
Copy link
Member Author

zroubalik commented Nov 23, 2021 •
edited by github-actions bot
Loading

/run-e2e
Update: You can check the progres here

@zroubalik
Copy link
Member Author

zroubalik commented Nov 23, 2021 •
edited by github-actions bot
Loading

/run-e2e
Update: You can check the progres here

@zroubalik
Copy link
Member Author

zroubalik commented Nov 23, 2021 •
edited by github-actions bot
Loading

/run-e2e
Update: You can check the progres here

Zbynek Roubalik added 7 commits November 23, 2021 14:27
allow setting MaxConcurrentReconciles for controllers
522dfc7 
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
set default scaledObjectMaxReconciles to 10
4f51b49 
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
init map
dc70502 
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
refactor
f8baca9 
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
update changelog
81e9f99 
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
util fn for ENV resolving
8a90166 
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
fix
670fd78 
Signed-off-by: Zbynek Roubalik <zroubali@redhat.com>
@zroubalik zroubalik merged commit 9ac7a0a into kedacore:main Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arschles arschles arschles requested changes

@JorTurFer JorTurFer JorTurFer approved these changes

@ahmelsayed ahmelsayed Awaiting requested review from ahmelsayed

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.5.0
Development

Successfully merging this pull request may close these issues.
4 participants
@zroubalik @JorTurFer @ahmelsayed @arschles