feat(sagemaker): add Endpoint L2 construct #22886
Conversation
This is the third and final PR to complete the implementation of RFC 431: aws/aws-cdk-rfcs#431 closes aws#2809 Co-authored-by: Matt McClean <[email protected]> Co-authored-by: Long Yao <[email protected]> Co-authored-by: Drew Jetter <[email protected]> Co-authored-by: Murali Ganesh <[email protected]> Co-authored-by: Abilash Rangoju <[email protected]>
|
|
github-actions
bot
added
beginning-contributor
There was a problem hiding this comment.
Okidoke @petermeansrock! I reviewed most of the code and left the tests alone for now. Few minor comments like always.
| * @param scalableProductionVariant ScalableProductionVariant instance | ||
| */ | ||
| private calculateScalingTarget( props: InvocationsScalingProps): number { | ||
| const safetyFactor = props.safetyFactor || 0.5; |
There was a problem hiding this comment.
are there any sane guardrails we should be putting on the safetyFactor? Is there a use case for someone to make safetyFactor > 1? Or 0? A few of these edge cases would result in calculateScalingTarget returning some problematic info, whether thats ridiculously high or low.
Co-authored-by: Kaizen Conroy <[email protected]>
mergify
bot
dismissed
kaizencc’s stale review
Pull request has been modified.
| productionVariant.metricInvocations().createAlarm(stack, 'InvocationsAlarm', { | ||
| threshold: 1, | ||
| evaluationPeriods: 2, | ||
| }); |
There was a problem hiding this comment.
last thing... these alarms don't seem to be needed since we're not asserting that they exist or anything. Lets move this to a separate integ test as it doesn't really test the endpoint construct. maybe integ.alarms.ts or something.
It is cluttering the current integ test atm.
There was a problem hiding this comment.
Adding changes requested since I'm looking for one thing in the integ test from an earlier comment. Everything else looks good, feel bad for blocking the approval on one thing but hopefully that's not too troublesome... once the integ tests are split out, I'm ready to slap the approval. Thanks for these quality PRs @petermeansrock!
mergify
bot
dismissed
kaizencc’s stale review
Pull request has been modified.
There was a problem hiding this comment.
Yay @petermeansrock, thanks for all your hard work and quick turn around time! This looks good to me, and means that your RFC design is fully implemented. Congrats! If you are willing/interested, it will be interesting to see what kind of usage + issues we get on this new L2 library and would be great to have your continued participation on those issues as they come in! I may try to ping you...
|
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). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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). |
This is the third and final PR to complete the implementation of RFC 431:
aws/aws-cdk-rfcs#431
closes #2809
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
Co-authored-by: Matt McClean [email protected]
Co-authored-by: Long Yao [email protected]
Co-authored-by: Drew Jetter [email protected]
Co-authored-by: Murali Ganesh [email protected]
Co-authored-by: Abilash Rangoju [email protected]