-
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(appmesh): add Connection Pools for VirtualNode and VirtualGateway #13917
Conversation
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.
Looks great @alexbrjo, love the fact that all of the options are type-safe, and you can't misuse the API!
A few small notes before we merge this in.
/** | ||
* Timeout for HTTP protocol | ||
* | ||
* @default - None | ||
*/ | ||
readonly timeout?: HttpTimeout; |
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.
Same note here - let's extract a common, module-private interface that extends VirtualNodeListenerCommonOptions
and contains the timeout
property, which both HttpVirtualNodeListenerOptions
and Http2VirtualNodeListenerOptions
can extend.
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.
They already all extend VirtualNodeListenerCommonOptions
, the timeout field is one of the ones that differs by protocol. Http
and Http2
are the only options out of 4 that share the same timeout so I'd like to leave this one.
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.
Why...?
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.
This all looks great @alexbrjo. If you could just get rid of the duplication in virtual-node-listener.ts
with the solution I outlined in #13917 (comment), we'll merge this in!
…irtualNodeListenerOptions and Http2VirtualNodeListenerOptions extend.
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.
@alexbrjo since this PR was so close, I made the changes myself, and pushed them to your branch. Let's get this merged in!
Thank you for contributing! Your pull request will be updated from master 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 master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
@skinny85 thanks so much for the help! |
aws#13917) Adds connection pools to VirtualNodes and VirtualGateways. VirtualNodes support listeners for HTTP, HTTP2, gRPC and TPC. VirtualGateways support only HTTP, HTTP2 and gRPC (no TCP). There are connection pool configurations for each of these 7 node/protocol combinations. * HTTP connection pools need these properties: maxConnections, maxPendingRequests * HTTP2 connection pools need these properties: maxRequests * gRPC connection pools need these properties: maxRequests * TCP connection pools need these properties: maxConnections Since connection pool properties differ by protocol so it's important to enforce the correct fields in CDK. To accomplish this I've added interfaces per protocol type and squashed API config `ConnectionPool > [Protocol] > properties` to the equivalent `ConnectionPool > properties` CDK config. This is ok because the user will never mismatch protocol types. From [docs (see connection pool section)](https://docs.aws.amazon.com/app-mesh/latest/userguide/virtual_gateways.html): > The connectionPool and portMapping protocols must be the same. If your listener protocol is grpc or http2, specify maxRequests only. If your listener protocol is http, you can specify both maxConnections and maxPendingRequests. ```ts new appmesh.VirtualNode(stack, 'test-node', { mesh, listeners: [ appmesh.VirtualNodeListener.tcp({ port: 80, connectionPool: { maxConnections: 100, }, }), ], }); ``` Pre-addressing the `ConnectionPoolConfig` placeholder interface & lint ignore statement: I’ve added this so I can treat the connection pool types as sub-classes while taking advantage of the interface object literal pattern. Cleaner UX compared to using classes. BREAKING CHANGE: HTTP2 `VirtualNodeListener`s must be now created with `Http2VirtualNodeListenerOptions` * **appmesh**: HTTP2 `VirtualGatewayListener`s must be now created with `Http2VirtualGatewayListenerOptions` Closes aws#11647 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Adds connection pools to VirtualNodes and VirtualGateways. VirtualNodes support listeners for HTTP, HTTP2, gRPC and TPC. VirtualGateways support only HTTP, HTTP2 and gRPC (no TCP). There are connection pool configurations for each of these 7 node/protocol combinations.
Since connection pool properties differ by protocol so it's important to enforce the correct fields in CDK. To accomplish this I've added interfaces per protocol type and squashed API config
ConnectionPool > [Protocol] > properties
to the equivalentConnectionPool > properties
CDK config. This is ok because the user will never mismatch protocol types. From docs (see connection pool section):Pre-addressing the
ConnectionPoolConfig
placeholder interface & lint ignore statement: I’ve added this so I can treat the connection pool types as sub-classes while taking advantage of the interface object literal pattern. Cleaner UX compared to using classes.BREAKING CHANGE: HTTP2
VirtualNodeListener
s must be now created withHttp2VirtualNodeListenerOptions
VirtualGatewayListener
s must be now created withHttp2VirtualGatewayListenerOptions
Closes #11647
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license