fix(ecs-patterns): handle desired task count being set to 0 #4722

nmussy · 2019-10-28T14:46:15Z

Allow desired task count to be set to 0 for the following (defaults to 1 when the property is undefined):

QueueProcessingServiceBase.desiredTaskCount property, affects:
- QueueProcessingEc2Service
- QueueProcessingFargateService
  Note: If desired task count is set to 0, maxScalingCapacity must be defined with a value greater than 0.

Throw exception when desired task count is set to 0 for the following (defaults to 1 when the property is undefined):

ApplicationLoadBalancedServiceBase.desiredCount property, affects:
- ApplicationLoadBalancedEc2Service
- ApplicationLoadBalancedFargateService
NetworkLoadBalancedServiceBase.desiredCount property, affects:
- NetworkLoadBalancedEc2Service
- NetworkLoadBalancedFargateService
ScheduledTaskBase.desiredTaskCount property, affects:
- ScheduledEc2Task
- ScheduledFargateTask

Fixes #4719

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

mergify · 2019-10-28T14:48:05Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

aws-cdk-automation · 2019-10-28T15:34:38Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

nmussy · 2019-10-28T16:24:27Z

@pkandasamy91 Your title is incorrect, this PR doesn't only cover queueprocessingservice, but also ApplicationLoadBalanced and NetworkLoadBalanced services

piradeepk

Thanks for taking the time to make this change. I don't think this follows the best practices for all patterns, and have added a couple comments

packages/@aws-cdk/aws-ecs-patterns/lib/base/application-load-balanced-service-base.ts

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

packages/@aws-cdk/aws-ecs-patterns/lib/base/network-load-balanced-service-base.ts

packages/@aws-cdk/aws-ecs-patterns/test/ec2/test.l3s.ts

nmussy · 2019-10-29T07:31:17Z

@pkandasamy91 We don't have to allow 0, seeing the anti-patterns you described in the issue.

This problem now is that is user isn't aware that they are not able to set a capacity to 0, and the CDK will silently bump it to 1. The way I see it, we can go about it two ways:

Allow 0, warn the user
Document the minimum value, throw if set to 0

What do you think?

piradeepk · 2019-10-29T15:30:22Z

@pkandasamy91 We don't have to allow 0, seeing the anti-patterns you described in the issue.

This problem now is that is user isn't aware that they are not able to set a capacity to 0, and the CDK will silently bump it to 1. The way I see it, we can go about it two ways:
* Allow 0, warn the user

* Document the minimum value, throw if set to 0
What do you think?

I definitely agree we need a way to notify users. I like the idea of adding to the documentation and throwing an exception. With a warning, it could easily be missed.

Pull request has been modified.

aws-cdk-automation · 2019-10-29T16:20:52Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

nmussy · 2019-10-30T08:22:50Z

@pkandasamy91 I also missed ScheduledTaskBase:

https://github.com/aws/aws-cdk/blob/master/packages/@aws-cdk/aws-ecs-patterns/lib/base/scheduled-task-base.ts#L108

Should we allow it like SQS, or throw like ALB/NLB?

aws-cdk-automation · 2019-10-30T09:12:47Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

piradeepk · 2019-10-30T16:12:05Z

@pkandasamy91 I also missed ScheduledTaskBase:

https://github.com/aws/aws-cdk/blob/master/packages/@aws-cdk/aws-ecs-patterns/lib/base/scheduled-task-base.ts#L108

Should we allow it like SQS, or throw like ALB/NLB?

I can't see a use case where a user would schedule a task to kick off with 0 desiredCount. For the time being I'm leaning towards throwing an exception. If a use case arises it can be changed after the fact as it wouldn't be a breaking change.

aws-cdk-automation · 2019-10-30T17:01:44Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

piradeepk · 2019-10-30T17:27:15Z

Please also add a note in your description for what the expected behaviour of each construct is (which ones throw vs which ones allow)

aws-cdk-automation · 2019-10-31T08:51:36Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2019-10-31T08:52:14Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

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

aws-cdk-automation · 2019-10-31T17:19:16Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

Pull request has been modified.

aws-cdk-automation · 2019-11-04T09:24:08Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

piradeepk

Thanks for taking the time to resolve this issue, and for addressing all of the feedback!

aws-cdk-automation · 2019-11-04T18:20:21Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2019-11-04T18:20:59Z

Thank you for contributing! Your pull request is now being automatically merged.

fix(ecs-patterns): allow 0 desired task count

6c09c8f

nmussy requested a review from SoManyHs as a code owner October 28, 2019 14:46

SomayaB added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label Oct 28, 2019

SomayaB assigned SoManyHs and piradeepk Oct 28, 2019

piradeepk unassigned SoManyHs Oct 28, 2019

piradeepk changed the title ~~fix(ecs-patterns): allow 0 desired task count~~ fix(ecs-patterns): allow queueprocessingservice to accept a desired task count of 0 Oct 28, 2019

piradeepk previously requested changes Oct 28, 2019

View reviewed changes

Merge branch 'master' into 4719

0b1f96e

Jimmy Gaussen added 3 commits October 30, 2019 09:00

Merge remote-tracking branch 'upstream/master' into 4719

2b750b2

fix: throw if alb/nlb pattern desiredCount < 0

9082da0

chore: move docs to props

e854920

chore: strict undefined check

ded879c

piradeepk changed the title ~~fix(ecs-patterns): allow queueprocessingservice to accept a desired task count of 0~~ fix(ecs-patterns): allow or throw an exception when desired task count is set to 0 Oct 30, 2019

Merge branch 'master' into 4719

34a527c

nmussy requested review from eladb and RomainMuller as code owners October 31, 2019 08:08

Merge remote-tracking branch 'upstream/master' into 4719

8843377

feat: throw if ScheduledTask desiredCount < 1

cc793ec

nmussy force-pushed the 4719 branch from 44d23c8 to cc793ec Compare October 31, 2019 08:15

Merge branch 'master' into 4719

1ea2ce8

piradeepk added the pr/do-not-merge This PR should not be merged at this time. label Oct 31, 2019

piradeepk changed the title ~~fix(ecs-patterns): allow or throw an exception when desired task count is set to 0~~ fix(ecs-patterns): handle desired task count being set to 0 Oct 31, 2019

piradeepk removed the pr/do-not-merge This PR should not be merged at this time. label Oct 31, 2019

piradeepk removed the request for review from RomainMuller October 31, 2019 16:52

piradeepk previously requested changes Oct 31, 2019

View reviewed changes

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

Jimmy Gaussen added 3 commits November 4, 2019 09:20

Merge remote-tracking branch 'upstream/master' into 4719

36d0c88

feat: check maxCapacity if not desiredCount

8398489

Merge branch '4719' of github.com:logicgram/aws-cdk into 4719

8bf8d46

Merge branch 'master' into 4719

b25264c

piradeepk approved these changes Nov 4, 2019

View reviewed changes

piradeepk added the @aws-cdk/aws-ecs-patterns Related to ecs-patterns library label Nov 4, 2019

mergify bot merged commit c31ca27 into aws:master Nov 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ecs-patterns): handle desired task count being set to 0 #4722

fix(ecs-patterns): handle desired task count being set to 0 #4722

nmussy commented Oct 28, 2019 •

edited by piradeepk

Loading

mergify bot commented Oct 28, 2019

aws-cdk-automation commented Oct 28, 2019

nmussy commented Oct 28, 2019

piradeepk left a comment

nmussy commented Oct 29, 2019

piradeepk commented Oct 29, 2019 •

edited

Loading

aws-cdk-automation commented Oct 29, 2019

nmussy commented Oct 30, 2019

aws-cdk-automation commented Oct 30, 2019

piradeepk commented Oct 30, 2019

aws-cdk-automation commented Oct 30, 2019

piradeepk commented Oct 30, 2019

aws-cdk-automation commented Oct 31, 2019

aws-cdk-automation commented Oct 31, 2019

aws-cdk-automation commented Oct 31, 2019

aws-cdk-automation commented Nov 4, 2019

piradeepk left a comment

aws-cdk-automation commented Nov 4, 2019

mergify bot commented Nov 4, 2019

fix(ecs-patterns): handle desired task count being set to 0 #4722

fix(ecs-patterns): handle desired task count being set to 0 #4722

Conversation

nmussy commented Oct 28, 2019 • edited by piradeepk Loading

mergify bot commented Oct 28, 2019

aws-cdk-automation commented Oct 28, 2019

AWS CodeBuild CI Report

nmussy commented Oct 28, 2019

piradeepk left a comment

Choose a reason for hiding this comment

nmussy commented Oct 29, 2019

piradeepk commented Oct 29, 2019 • edited Loading

aws-cdk-automation commented Oct 29, 2019

AWS CodeBuild CI Report

nmussy commented Oct 30, 2019

aws-cdk-automation commented Oct 30, 2019

AWS CodeBuild CI Report

piradeepk commented Oct 30, 2019

aws-cdk-automation commented Oct 30, 2019

AWS CodeBuild CI Report

piradeepk commented Oct 30, 2019

aws-cdk-automation commented Oct 31, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 31, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 31, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 4, 2019

AWS CodeBuild CI Report

piradeepk left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Nov 4, 2019

AWS CodeBuild CI Report

mergify bot commented Nov 4, 2019

nmussy commented Oct 28, 2019 •

edited by piradeepk

Loading

piradeepk commented Oct 29, 2019 •

edited

Loading