feat(ecs-patterns): support disabling CPU-based scaling and custom target utilization by AnuragMohapatra · Pull Request #28315 · aws/aws-cdk

AnuragMohapatra · 2023-12-09T08:45:15Z

Added an optional parameter that defaults to false over the CPU-based scaling policy that conflicts with the queue visible message-based policy.

When disabled this will stop the race condition issue mentioned in #20706 by only allowing the scaling of the number of messages on the queue similar to the SQS-Lambda pattern.

Note: If this parameter is enabled then this bug will crop up again and the user has to handle the container termination manually.

Updated integration tests and unit tests are working.

Closes #20706 .

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

AnuragMohapatra · 2023-12-09T08:58:03Z

Exemption Request : There is no integration tests for the library

lpizzinidev

Thanks for the contribution! 👍
I left a couple of comments for improvements.

Notes:

This PR should be a feat: feat(ecs-patterns): support disabling CPU-based scaling and custom target utilization
You should add an integration test for the new feature (you can use this as a blueprint, see guidelines on how to create/update one)

packages/aws-cdk-lib/aws-ecs-patterns/lib/base/queue-processing-service-base.ts

packages/aws-cdk-lib/aws-ecs-patterns/README.md

packages/aws-cdk-lib/aws-ecs-patterns/test/fargate/queue-processing-fargate-service.test.ts

Main

…pscalingstrategyracecondition Fix/issue 20706 allowtostopscalingstrategyracecondition

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

AnuragMohapatra · 2023-12-15T13:36:48Z

@lpizzinidev All review comments have been addressed.

I have found the integration tests are broken at the moment and an issue has been raised - #28383

I fixed it temporarily to get the current integ running but it does not look promising for some test cases.

…acecondition

…https://github.com/AnuragMohapatra/aws-cdk into fix/issue-20706-fixforcpubasedautoscaleracecondition

…ondition

…https://github.com/AnuragMohapatra/aws-cdk into fix/issue-20706-fixforcpubasedautoscaleracecondition

lpizzinidev

Thanks for the changes 👍
I left other notes for some cleanup.
Please note the comment I left before:

This PR should be a feat: feat(ecs-patterns): support disabling CPU-based scaling and custom target utilization

packages/aws-cdk-lib/aws-ecs-patterns/README.md

packages/aws-cdk-lib/aws-ecs-patterns/lib/base/queue-processing-service-base.ts

packages/aws-cdk-lib/aws-ecs-patterns/test/fargate/queue-processing-fargate-service.test.ts

packages/aws-cdk-lib/aws-ecs-patterns/README.md

AnuragMohapatra · 2023-12-17T00:41:18Z

@lpizzinidev Thanks for the review and updated all requested changes.

lpizzinidev · 2023-12-19T09:09:57Z

@AnuragMohapatra
Please remove the double feat from the title: feat(ecs-patterns) ...

…ondition

kaizencc

Thanks for your PR and integ test @AnuragMohapatra. I have a few comments, and also please update the PR description to reflect the fact that an integ test here exists!

packages/aws-cdk-lib/aws-ecs-patterns/README.md

packages/aws-cdk-lib/aws-ecs-patterns/lib/base/queue-processing-service-base.ts

…https://github.com/AnuragMohapatra/aws-cdk into fix/issue-20706-fixforcpubasedautoscaleracecondition

Pull request has been modified.

…ondition

…https://github.com/AnuragMohapatra/aws-cdk into fix/issue-20706-fixforcpubasedautoscaleracecondition

…ondition

packages/aws-cdk-lib/aws-ecs-patterns/lib/base/queue-processing-service-base.ts

kaizencc

Thanks @AnuragMohapatra

kaizencc

Oops :)

mergify · 2023-12-22T22:19:46Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…ondition

aws-cdk-automation · 2023-12-22T23:29:17Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: dd0b470
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-12-22T23:29:40Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

AnuragMohapatra · 2023-12-23T00:36:15Z

@kaizencc I think this also closed the issue #23624 but I was not aware this was there when I was adding the target CPU utilization property.

…rget utilization (aws#28315) Added an optional parameter that defaults to false over the CPU-based scaling policy that conflicts with the queue visible message-based policy. When disabled this will stop the race condition issue mentioned in aws#20706 by only allowing the scaling of the number of messages on the queue similar to the SQS-Lambda pattern. Note: If this parameter is enabled then this bug will crop up again and the user has to handle the container termination manually. Updated integration tests and unit tests are working. Closes aws#20706 . ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Adding property to allow consumer to disable Cpu based scaling

81d3235

aws-cdk-automation requested a review from a team December 9, 2023 08:45

github-actions bot added bug This issue is a bug. p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Dec 9, 2023

aws-cdk-automation previously requested changes Dec 9, 2023

View reviewed changes

AnuragMohapatra changed the title ~~fix(aws-ecs-patterns): Adding property to allow consumer to disable Cpu based scaling~~ fix(ecs-patterns): Adding property to allow consumer to disable Cpu based scaling Dec 9, 2023

aws-cdk-automation added pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Dec 9, 2023

lpizzinidev suggested changes Dec 10, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 10, 2023

AnuragMohapatra and others added 3 commits December 15, 2023 13:26

Fix PR review comments

918174d

Merge pull request #1 from AnuragMohapatra/main

b4da8f6

Main

Merge pull request #2 from AnuragMohapatra/fix/issue-20706-allowtosto…

785ed03

…pscalingstrategyracecondition Fix/issue 20706 allowtostopscalingstrategyracecondition

AnuragMohapatra and others added 7 commits December 15, 2023 22:18

Fix integration testing

2606c19

Merge branch 'aws:main' into fix/issue-20706-fixforcpubasedautoscaler…

5d16287

…acecondition

Merge branch 'fix/issue-20706-fixforcpubasedautoscaleracecondition' of …

8a08738

…https://github.com/AnuragMohapatra/aws-cdk into fix/issue-20706-fixforcpubasedautoscaleracecondition

Fix the integration test failure

1476dc0

Merge branch 'main' into fix/issue-20706-fixforcpubasedautoscaleracec…

1161013

…ondition

Fix typo in readme

41cfbee

Merge branch 'fix/issue-20706-fixforcpubasedautoscaleracecondition' of …

fecca49

…https://github.com/AnuragMohapatra/aws-cdk into fix/issue-20706-fixforcpubasedautoscaleracecondition

lpizzinidev suggested changes Dec 16, 2023

View reviewed changes

Update clean ups as per PR review

ca9390b

AnuragMohapatra changed the title ~~fix(ecs-patterns): Adding property to allow consumer to disable Cpu based scaling~~ feat: feat(ecs-patterns): support disabling CPU-based scaling and custom target utilization Dec 17, 2023

AnuragMohapatra added 2 commits December 17, 2023 00:51

fixing linting issues

4edfc9e

Fix indentation

419a956

lpizzinidev approved these changes Dec 19, 2023

View reviewed changes

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 19, 2023

AnuragMohapatra changed the title ~~feat: feat(ecs-patterns): support disabling CPU-based scaling and custom target utilization~~ feat(ecs-patterns): support disabling CPU-based scaling and custom target utilization Dec 19, 2023

Merge branch 'main' into fix/issue-20706-fixforcpubasedautoscaleracec…

ea0fcf3

…ondition

kaizencc previously requested changes Dec 19, 2023

View reviewed changes

kaizencc removed the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Dec 19, 2023

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 19, 2023

Update readme

a6b1113

AnuragMohapatra commented Dec 20, 2023

View reviewed changes

packages/aws-cdk-lib/aws-ecs-patterns/lib/base/queue-processing-service-base.ts Show resolved Hide resolved

Merge branch 'fix/issue-20706-fixforcpubasedautoscaleracecondition' of …

1d20c8e

…https://github.com/AnuragMohapatra/aws-cdk into fix/issue-20706-fixforcpubasedautoscaleracecondition

Merge branch 'main' into fix/issue-20706-fixforcpubasedautoscaleracec…

80db0af

…ondition

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 20, 2023

AnuragMohapatra and others added 3 commits December 21, 2023 11:02

make the properties private in base class

1d5cbbd

Merge branch 'fix/issue-20706-fixforcpubasedautoscaleracecondition' of …

8a2d795

…https://github.com/AnuragMohapatra/aws-cdk into fix/issue-20706-fixforcpubasedautoscaleracecondition

Merge branch 'main' into fix/issue-20706-fixforcpubasedautoscaleracec…

d544f31

…ondition

kaizencc reviewed Dec 22, 2023

View reviewed changes

packages/aws-cdk-lib/aws-ecs-patterns/lib/base/queue-processing-service-base.ts Outdated Show resolved Hide resolved

packages/aws-cdk-lib/aws-ecs-patterns/lib/base/queue-processing-service-base.ts Outdated Show resolved Hide resolved

Apply suggestions from code review

33efe97

kaizencc reviewed Dec 22, 2023

View reviewed changes

kaizencc approved these changes Dec 22, 2023

View reviewed changes

Merge branch 'main' into fix/issue-20706-fixforcpubasedautoscaleracec…

dd0b470

…ondition

mergify bot merged commit 3cb3e02 into aws:main Dec 22, 2023

AnuragMohapatra mentioned this pull request Dec 23, 2023

aws-ecs-patterns (QueueProcessingFargateService): Make Target CPU Utilization configurable #23624

Closed

2 tasks

github-actions bot mentioned this pull request Jan 1, 2024

Monthly PR metrics report - Dec 2023 #28544

Closed

Conversation

AnuragMohapatra commented Dec 9, 2023 • edited by kaizencc Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

AnuragMohapatra commented Dec 9, 2023

Uh oh!

lpizzinidev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AnuragMohapatra commented Dec 15, 2023

Uh oh!

lpizzinidev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AnuragMohapatra commented Dec 17, 2023

Uh oh!

lpizzinidev commented Dec 19, 2023

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Dec 22, 2023

Uh oh!

aws-cdk-automation commented Dec 22, 2023

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Dec 22, 2023

Uh oh!

AnuragMohapatra commented Dec 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

AnuragMohapatra commented Dec 9, 2023 •

edited by kaizencc

Loading