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

Refactoring concurrency in scaler's pending HTTP queue fetcher logic #291

Merged
merged 14 commits into from
Oct 25, 2021

Conversation

arschles
Copy link
Collaborator

@arschles arschles commented Oct 7, 2021

This patch refactors the queuePinger's requestCounts function in the scaler. Highlights:

  • Renames requestCounts to fetchAndSaveCounts
  • Creates a new fetchCounts function (not method on the queuePinger) that does all the heavy lifting
  • Changes fetchAndSaveCounts to simply call fetchCounts and save the results internally to the queuePinger
  • Adds tests for fetchCounts
  • Ensures that all counts are fetched when fetchCounts returns
  • The queue tick duration is now configurable

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO)
  • Any necessary documentation is added, such as:
  • Refactor requestCounts to update all counts before it returns
  • Write tests that call requestCounts directly
  • The queue pinger should not start the background goroutine in its constructor. A separate function call should be required for that.

Fixes #286
Fixes #139

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>
Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>
@arschles arschles added bug Something isn't working testing labels Oct 7, 2021
@arschles arschles added this to the v0.2.0 milestone Oct 7, 2021
@arschles
Copy link
Collaborator Author

arschles commented Oct 7, 2021

cc/ @asw101

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>
Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>
Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>
Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>
Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>
Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>
…erver

Signed-off-by: Aaron Schlesinger <aaron@ecomaz.net>
…equest-counts-concurrency

Signed-off-by: Aaron <aaron@ecomaz.net>
@arschles arschles enabled auto-merge (squash) October 19, 2021 23:31
Copy link

@bketelsen bketelsen 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 disabled auto-merge October 25, 2021 18:33
…equest-counts-concurrency

Signed-off-by: Aaron <aaron@ecomaz.net>
@arschles
Copy link
Collaborator Author

Thanks @bketelsen !

Although the linkinator CI check is failing, I'm going to merge this anyway. The failure is due to kedacore/keda#2215, and the work to fix it will be done separately.

@arschles arschles merged commit 3df5d09 into kedacore:main Oct 25, 2021
@arschles arschles deleted the request-counts-concurrency branch October 25, 2021 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor concurrency in scaler requestCounts function Add tests for the scaler queue pinger
2 participants