-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(appsync): set max batch size when using batch invoke #20995
Conversation
@@ -112,6 +118,7 @@ export class Resolver extends Construct { | |||
requestMappingTemplate: props.requestMappingTemplate ? props.requestMappingTemplate.renderTemplate() : undefined, | |||
responseMappingTemplate: props.responseMappingTemplate ? props.responseMappingTemplate.renderTemplate() : undefined, | |||
cachingConfig: this.createCachingConfig(props.cachingConfig), | |||
maxBatchSize: props.maxBatchSize, |
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.
Should it not be undefined
if it's not specified?
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.
Yes, will change it.
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.
If props.maxBatchSize
isn't specified, it would be undefined with the code as is. maxBatchSize: props.maxBatchSize ? props.maxBatchSize : undefined;
is redundant, unless there's something I'm missing.
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.
@TheRealAmazonKendra
You are probably right (I am not too familiar with typescript). I think there is no drawback if it is checked. Actually, it adds some consistency, since other variables are checked as well (line 115 probs.dataSourceName
).
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.
It's checked in line 115 because it's setting dataSourceName
to props.dataSource.name
. In this case, it would throw an error if props.dataSource
is undefined.
The whole line: dataSourceName: props.dataSource ? props.dataSource.name : undefined,
That check is just redundant here, like it would be in lines 112-114.
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.
I have missed that. I will just revert it, removing the redundant call.
@TheRealAmazonKendra, am I required to add this single field to the |
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.
Thank you for your contribution!
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). |
closes #20467
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integ
to deploy the infrastructure and generate the snapshot (i.e.yarn integ
without--dry-run
)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license