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

fix: Adapter has high qps to apiserver #2922

Conversation

bamboo12366
Copy link
Contributor

@bamboo12366 bamboo12366 commented Apr 20, 2022

Provide a description of what has been changed

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • [N/A] Tests have been added
  • [N/A] 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)
  • Changelog has been updated and is aligned with our changelog requirements

Fixes #2914

@zroubalik
Copy link
Member

zroubalik commented Apr 20, 2022

Looking good, could you please also pass the same client to the handler on line 106? So we can get rid of the kubeclient completely?

Also, have you been able to test this setup to see if it really helped with the issue?

@bamboo12366
Copy link
Contributor Author

bamboo12366 commented Apr 20, 2022

could you please also pass the same client to the handler on line 106? So we can get rid of the kubeclient completely?

will do later

have you been able to test this setup to see if it really helped with the issue?

I test the image locally, only the first request will hit the apiserver, after there are only list from informer cache(guaranteed by list-watch)

@zroubalik
Copy link
Member

Cool :)

Then please also fix DCO and update the Changelog, that's a great improvement!

@bamboo12366 bamboo12366 changed the title Bamboo/fix adapter high qps to apiserver fix adapter high qps to apiserver Apr 25, 2022
@bamboo12366 bamboo12366 force-pushed the bamboo/fix-adapter-high-qps-to-apiserver branch from 337f0ab to 3b034e6 Compare April 25, 2022 10:57
@bamboo12366
Copy link
Contributor Author

@zroubalik Can I just skip the test case in this PR? Since this one only mitigate the burden to Apiserver, will not affect the performance/logic in Keda side

CHANGELOG.md Outdated Show resolved Hide resolved
adapter/main.go Show resolved Hide resolved
@zroubalik
Copy link
Member

@zroubalik Can I just skip the test case in this PR? Since this one only mitigate the burden to Apiserver, will not affect the performance/logic in Keda side

@bamboo12366 yeah, no need for test in this case.

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
Awesome improvement! ❤️
Only 2 small nits inline

CHANGELOG.md Outdated Show resolved Hide resolved
adapter/main.go Outdated Show resolved Hide resolved
@zroubalik
Copy link
Member

zroubalik commented Apr 25, 2022

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

@JorTurFer
Copy link
Member

JorTurFer commented Apr 26, 2022

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

CHANGELOG.md Outdated Show resolved Hide resolved
@JorTurFer
Copy link
Member

JorTurFer commented Apr 26, 2022

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

@JorTurFer
Copy link
Member

JorTurFer commented Apr 26, 2022

I'm not sure how, but it seems that these changes are breaking e2e tests. They have failed 3 times in a row when in main branch they are working. Maybe e2e tests are based on this wrong approach, I'll take a look deeper asap

@JorTurFer
Copy link
Member

JorTurFer commented Apr 26, 2022

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

@tomkerkhove tomkerkhove changed the title fix adapter high qps to apiserver fix: Adapter has high qps to apiserver Apr 26, 2022
@zroubalik
Copy link
Member

zroubalik commented Apr 27, 2022

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

@zroubalik
Copy link
Member

zroubalik commented Apr 27, 2022

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

@JorTurFer
Copy link
Member

Hey @bamboo12366
We plan to do a release during next week. I have solved the problems with e2e tests (they were in other places, not in your changes), could you rebase your branch and update the changelog? 🙏
I'll trigger e2e test again and once they pass, we can merge it to include this improvement in next release

@bamboo12366
Copy link
Contributor Author

@JorTurFer I update CHANLOG and resolve conflicts accordingly, could you please help me review again to see if everything fit?

@JorTurFer
Copy link
Member

JorTurFer commented Apr 30, 2022

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

Failure is expected, but only graphite scaler

@JorTurFer
Copy link
Member

JorTurFer commented Apr 30, 2022

I'm not sure about what commit breaks the DCO check @bamboo12366, but at least 1 commit is not signed.
Could you squash your branch for just pushing your commits (signed)?

@bamboo12366
Copy link
Contributor Author

sorry I am not that familiar with git. I already merge the rebase and merge the main into current branch, I don't know how to squash commit now, could you advise more? @JorTurFer

@JorTurFer
Copy link
Member

sorry I am not that familiar with git. I already merge the rebase and merge the main into current branch, I don't know how to squash commit now, could you advise more? @JorTurFer

Sure, you can do this:

git checkout yourBranch
git reset $(git merge-base main $(git branch --show-current))
git add -A
git commit -sm "one commit on yourBranch"

Squash the branch is not mandatory, but it's the easiest way to ensure all commits are signed (because only 1 needs to be signed xD)

Signed-off-by: Xinzhu Zhou <bamboo12366@gmail.com>
@bamboo12366 bamboo12366 force-pushed the bamboo/fix-adapter-high-qps-to-apiserver branch from f861892 to d5e7d7f Compare April 30, 2022 15:18
@bamboo12366
Copy link
Contributor Author

sorry I am not that familiar with git. I already merge the rebase and merge the main into current branch, I don't know how to squash commit now, could you advise more? @JorTurFer

Sure, you can do this:

git checkout yourBranch
git reset $(git merge-base main $(git branch --show-current))
git add -A
git commit -sm "one commit on yourBranch"

Squash the branch is not mandatory, but it's the easiest way to ensure all commits are signed (because only 1 needs to be signed xD)

Thanks A Lot!! You truly save my live!

@JorTurFer
Copy link
Member

JorTurFer commented Apr 30, 2022

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

@JorTurFer JorTurFer merged commit c60acf5 into kedacore:main May 2, 2022
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.

keda-metrics-adapter calling apiserver qps are quite high
3 participants