-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
AWS SQS: Respect scaleOnDelayed #4383
Conversation
Signed-off-by: Ivan Ding <ivan.ding@grabtaxi.com>
Signed-off-by: Ivan Ding <ivan.ding@grabtaxi.com>
Signed-off-by: Ivan Ding <ivan.ding@grabtaxi.com>
Signed-off-by: Ivan Ding <ivan.ding@grabtaxi.com>
Signed-off-by: Ivan Ding <ivan.ding@grabtaxi.com>
Signed-off-by: Ivan Ding <ivan.ding@grabtaxi.com>
Signed-off-by: Ivan Ding <ivan.ding@grabtaxi.com>
There was a problem hiding this 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 the improvement ❤️
Some nits inline
@@ -53,6 +53,7 @@ To learn more about active deprecations, we recommend checking [GitHub Discussio | |||
|
|||
### Fixes | |||
|
|||
- **AWS SQS Scaler**: Respect `scaleOnDelayed` value ([#4377](https://github.com/kedacore/keda/issue/4377)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move this to New
section? This isn't a fix because it's a new feature. The message should be something like Add support to scaledOnDelayed
"ApproximateNumberOfMessages", | ||
"ApproximateNumberOfMessagesNotVisible", | ||
} | ||
|
||
var awsSqsQueueMetricNamesForNotScalingInFlight = []string{ | ||
var awsSqsQueueMetricNamesForScaling = []string{ | ||
"ApproximateNumberOfMessages", | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does keeping this here make sense? I mean, maybe we could just move it to the parsing function
@ding1992 any update on this please? |
I'm very interested in this functionality. @ding1992 any updates at all? Anything I can help with? |
@JorTurFer Yes, I'll give it a go! |
I linked to this PR in my new PR, but just for clarity, here is the new PR #4900 |
already done by #4900 |
Provide a description of what has been changed
Checklist
Fixes #4377