Skip to content
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: support orchestrator #1435

Merged
merged 1 commit into from
May 8, 2024
Merged

feat: support orchestrator #1435

merged 1 commit into from
May 8, 2024

Conversation

milesziemer
Copy link
Contributor

@milesziemer milesziemer commented Apr 9, 2024

Updates the codegenerator and client libraries to enable using Orchestrator instead of OperationStack for operations. It is currently enabled for all protocol tests, and some services with integration tests. We can enable it for more services in the future.

The codegen updates to support Orchestrator do change the code for services not using Orchestrator, but its just refactoring (EndpointResolverMiddleware for example).

See Orchestrator impl in smithy-lang/smithy-swift#694

For testing, I ran protocol tests and integration tests with interceptors enabled and disabled.

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

@milesziemer milesziemer force-pushed the interceptors-impl branch 2 times, most recently from e86e753 to c019f22 Compare April 25, 2024 21:52
@milesziemer milesziemer marked this pull request as ready for review April 25, 2024 21:53
@milesziemer milesziemer force-pushed the interceptors-impl branch 4 times, most recently from 3d35527 to 9b05b97 Compare April 30, 2024 14:39
Copy link
Contributor

@syall syall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and feeling confident if CI passes.

I did leave some comments for some middleware that I thought would have to implement an orchestrator runtime component (especially for S3, and not exhaustive).

Comment on lines +50 to +54
override fun renderMiddlewareInit(
ctx: ProtocolGenerator.GenerationContext,
writer: SwiftWriter,
op: OperationShape
) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does FlexibleChecksumsResponseIntegration need a branch for useInterceptors? (e.g. for S3)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +53 to +57
override fun renderMiddlewareInit(
ctx: ProtocolGenerator.GenerationContext,
writer: SwiftWriter,
op: OperationShape
) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does FlexibleChecksumsRequestIntegration need a branch for useInterceptors? (e.g. for S3)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little roundabout how this works, but to render a MiddlewareRenderable, render gets called, which by default handles useInterceptors itself, and calls renderMiddlewareInit. Most middleware initialization works exactly the same whether using interceptors or not, so I added some abstraction to MiddlewareRenderable in the smithy-swift PR: https://github.com/smithy-lang/smithy-swift/pull/694/files#diff-60728c54157d6f8ad167a46845e0c2c47bc5abb02644411b20f5e5102a113942R48-R70

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@milesziemer so does that mean FlexibleChecksumsRequestIntegration doesn't need a branch for useInterceptors because render will be called anyway which already goes down that branch? Additionally, should this be refactored into an interceptor instead of a middleware?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so does that mean FlexibleChecksumsRequestIntegration doesn't need a branch for useInterceptors because render will be called anyway which already goes down that branch?

Yes

Additionally, should this be refactored into an interceptor instead of a middleware?

If you mean the swift implementation, it is, just in smithy-lang/smithy-swift#694

Comment on lines +60 to +64
override fun renderMiddlewareInit(
ctx: ProtocolGenerator.GenerationContext,
writer: SwiftWriter,
op: OperationShape
) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does S3ErrorWith200StatusIntegration need a branch for useInterceptors? (e.g. for S3)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@milesziemer milesziemer force-pushed the interceptors-impl branch 2 times, most recently from 8759528 to 55026cb Compare May 1, 2024 17:03
@syall syall mentioned this pull request May 6, 2024
4 tasks
@milesziemer milesziemer force-pushed the interceptors-impl branch from 55026cb to 72d9745 Compare May 7, 2024 15:52
Updates the codegenerator and client libraries to enable using Orchestrator
instead of OperationStack for operations. It is currently enabled for
all protocol tests, and some services with integration tests. We can enable
it for more services in the future.

The codegen updates to support Orchestrator do change the code for services
not using Orchestrator, but its just refactoring (EndpointResolverMiddleware
for example).
@milesziemer milesziemer force-pushed the interceptors-impl branch from 72d9745 to 7410996 Compare May 8, 2024 16:45
@milesziemer milesziemer merged commit 433b2e5 into main May 8, 2024
29 checks passed
@milesziemer milesziemer deleted the interceptors-impl branch May 8, 2024 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants