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

Use approximate queue length for azure queue scaler #4003

Merged
merged 6 commits into from
Feb 28, 2023

Conversation

moria97
Copy link
Contributor

@moria97 moria97 commented Dec 12, 2022

Use the approximate queue length instead of visible messages, see #4002

Checklist

Fixes #4002

Relates to #

Signed-off-by: Yue Fei <yufei@microsoft.com>
@moria97 moria97 force-pushed the personal/yufei/fi-queue-length branch from 8d0366e to d18575f Compare December 12, 2022 12:59
@moria97 moria97 marked this pull request as ready for review December 13, 2022 04:15
@moria97 moria97 requested a review from a team as a code owner December 13, 2022 04:15
@moria97 moria97 changed the title Use approximate queue length Use approximate queue length for azure queue scaler Dec 13, 2022
@tomkerkhove
Copy link
Member

By removing the visible count you are not actually achieving what you are suggesting in #4002

@moria97
Copy link
Contributor Author

moria97 commented Jan 3, 2023

By removing the visible count you are not actually achieving what you are suggesting in #4002

Removing the visible count will return the approximate message count directly, which contains messages that are queued and being processed. Not sure if there is anything wrong? @tomkerkhove

Copy link
Contributor

@v-shenoy v-shenoy left a comment

Choose a reason for hiding this comment

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

LGTM

@v-shenoy
Copy link
Contributor

v-shenoy commented Jan 3, 2023

/run-e2e azure_queue
Update: You can check the progress here

@tomkerkhove
Copy link
Member

I have misread the issue, no worries.

@tomkerkhove
Copy link
Member

Can you add an entry in the changelog please?

I've pinged @ahmelsayed privately as well to see if we can remember why we added that.

Signed-off-by: Yue Fei <59813791+moria97@users.noreply.github.com>
@tomkerkhove
Copy link
Member

@ahmelsayed pointed out that this was changed in #2613 by @RamCohen as an optimization.

I think the optimization is still OK to have and we can skip this PR but I don't have a strong opinion.

@moria97
Copy link
Contributor Author

moria97 commented Jan 4, 2023

@tomkerkhove This optimization looks make sense to me. But the issue is not related to the optimization, but the visible message count used to count queue length.

Let's say "queue length" parameter is 1, there are 20 messages and scaled out to 20 instances and pulled message from queue, the queue size returned is 0 (visible message account), but the actual value should be 20 (approximate queue length).

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.

@moria97 @tomkerkhove @v-shenoy so what is the status of this PR?

@antonio-policastro
Copy link

antonio-policastro commented Feb 27, 2023

I'm shipping a custom version of this scaler in production since v2.6.1. My custom versions have the exact same code as in this pull request, and they have been working perfectly fine for the past year. ApproximateMessageCount is the way to go.
There are multiple issues about this problem(eg: #2385 #3555).

The optimization done with #2613 will be useless, because we should always call GetProperties to get the ApproximateMessageCount.

I hope this will be merged soon.
Thanks

Signed-off-by: Yue Fei <59813791+moria97@users.noreply.github.com>
@moria97
Copy link
Contributor Author

moria97 commented Feb 28, 2023

@tomkerkhove @zroubalik Can you take a look at this pr?

Copy link
Contributor

@v-shenoy v-shenoy left a comment

Choose a reason for hiding this comment

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

I approved this earlier also. I think we can (should) merge this.

@tomkerkhove @JorTurFer

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

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

tomkerkhove commented Feb 28, 2023

/run-e2e azure_queue

Update: You can check the progress here

Signed-off-by: Yue Fei <59813791+moria97@users.noreply.github.com>
Signed-off-by: Yue Fei <59813791+moria97@users.noreply.github.com>
@JorTurFer
Copy link
Member

JorTurFer commented Feb 28, 2023

/run-e2e azure_queue*
Update: You can check the progress here

@JorTurFer JorTurFer enabled auto-merge (squash) February 28, 2023 12:27
@JorTurFer JorTurFer merged commit 0ba2313 into kedacore:main Feb 28, 2023
@wallflower762
Copy link

Thanks team, this is great!

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.

Azure Storage Queue Scaler: should report both visible and invisible message count to compute queue length.
7 participants