design(aws-ecs) Design proposal for Queue Worker Service L3 construct

[piradeepk]

Fixes #2396

---

Pull Request Checklist

• Testing
  • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
  • CLI change?: coordinate update of integration tests with team
  • cdk-init template change?: coordinated update of integration tests with team
• Docs
  • jsdocs: All public APIs documented
  • README: README and/or documentation topic updated
  • Design: For significant features, design document added to design folder
• Title and Description
  • Change type: title prefixed with fix, feat and module name in parens, which will appear in changelog
  • Title: use lower-case and doesn't end with a period
  • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
  • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
• Sensitive Modules (requires 2 PR approvers)
  • IAM Policy Document (in @aws-cdk/aws-iam)
  • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
  • Grant APIs (only if not based on official documentation with a reference)

---

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

[piradeepk]

Open question: The scaling factor for the ApproximateNumberOfMessagesVisible metric will vary based on how fast a message can be processed by the container. Does it make sense to set a default and make it on the higher end and provide the option to pass in a custom value as part of the props? Would requiring the property be better?

Example:
scalingSteps: [
{ upper: 100, change: -1 },
{ lower: 500, change: +1 },
{ lower: 700, change: +3 },
]

[clareliguori]

@pkandasamy91 Other consideration is how latency-sensitive the application is, so 200 messages may not be a big deal and not need scaling if the application's SLA for processing messages is 5 minutes. I think it makes sense to set a default and allow a property.

Side note, is there a minimum scaling set? Is it equal to the desired count?

[nathanpeck]

I actually would only decrease the number of workers only if the approximate number of messages visible is zero.

[clareliguori]

@nathanpeck Can you suggest a default step scaling policy for what you're suggesting?

[piradeepk]

@pkandasamy91 Other consideration is how latency-sensitive the application is, so 200 messages may not be a big deal and not need scaling if the application's SLA for processing messages is 5 minutes. I think it makes sense to set a default and allow a property.

Side note, is there a minimum scaling set? Is it equal to the desired count?

Yes, the minimum scaling capacity is set to the desired task count (default 1 if not provided)

[piradeepk]

I've added a scalingSteps prop and provided a default that seems reasonable (spoke to @nathanpeck and confirmed that it's a good starting point). Doc has been updated to address all other feedback as well

[SoManyHs]

The interfaces generally seem fine -- I'm not super familiar with the SQS construct lib so will have a better feel for this when it's implemented but besides making CPU part of the common props no major changes; most comments more about writing style/clarity.

[piradeepk]

Squashed commits

[BDQ]

Would something like this not be better suited to a third party module? Why does it need to be cdk itself?

I'd be wary of accepting lots of different L3 patterns for services, when there's a perfectly valid method of packaging them as independent modules, seems like it could explode an already sizable code base.

[eladb]

I tend to agree with @BDQ that these shouldn't go into the aws-ecs module. These are still 1st party and should be shipped together with the CDK but I think we should put these L3s under a new module. This is something @clareliguori and I also discussed. How about we move all the L3 patterns into a new module called @aws-cdk/aws-ecs-plus (we were discussing that we can use the "plus" "branding" to indicate those are higher level modules.

@rix0rrr @fulghum what do you guys think?

Thoughts?

[SoManyHs]

This should be encapsulated in #2568. Closing.