-
Notifications
You must be signed in to change notification settings - Fork 187
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
Fix: orchestrator flow #2699
Fix: orchestrator flow #2699
Conversation
I still have some rough edges to sand off, but the interceptor tests are passing. |
|
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.
Very nice work on this! It's a huge improvement.
rust-runtime/aws-smithy-runtime-api/src/client/interceptors/context/wrappers.rs
Outdated
Show resolved
Hide resolved
update: interceptor tests to work with the new wrappers.
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
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.
Overall, I really like these changes! Great work on this.
aws/sra-test/integration-tests/aws-sdk-s3/tests/interceptors.rs
Outdated
Show resolved
Hide resolved
rust-runtime/aws-smithy-runtime-api/src/client/interceptors/context.rs
Outdated
Show resolved
Hide resolved
rust-runtime/aws-smithy-runtime-api/src/client/interceptors/context.rs
Outdated
Show resolved
Hide resolved
rust-runtime/aws-smithy-runtime-api/src/client/interceptors/context/wrappers.rs
Show resolved
Hide resolved
@@ -61,6 +62,11 @@ impl InterceptorError { | |||
interceptor_error_fn!(modify_before_completion => ModifyBeforeCompletion (with source)); | |||
interceptor_error_fn!(read_after_execution => ReadAfterExecution (with source)); | |||
|
|||
interceptor_error_fn!(modify_before_attempt_completion_failed => ModifyBeforeAttemptCompletion (with source)); |
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.
Are these still needed?
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, because even though an error in these interceptors won't cause us to return, we still need to record that an interceptor failed.
rust-runtime/aws-smithy-runtime-api/src/client/interceptors/error.rs
Outdated
Show resolved
Hide resolved
A new generated diff is ready to view.
A new doc preview is ready to view. |
remove: `impl From<InterceptorError> for TypeErasedError` formatting: update the `halt_on_err` macro to fmt nicely
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
A new generated diff is ready to view.
A new doc preview is ready to view. |
## Motivation and Context <!--- Why is this change required? What problem does it solve? --> <!--- If it fixes an open issue, please link to the issue here --> Necessary before we can implement retries. ## Description <!--- Describe your changes in detail --> I noticed that we weren't handling the flow quite right when errors occurred. This PR fixes that and adds interceptor-based tests to ensure things are working right. I still think we could use more tests but the PR is already quite large. ## Testing <!--- Please describe in detail how you tested your changes --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> This PR contains tests ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._
## Motivation and Context <!--- Why is this change required? What problem does it solve? --> <!--- If it fixes an open issue, please link to the issue here --> Necessary before we can implement retries. ## Description <!--- Describe your changes in detail --> I noticed that we weren't handling the flow quite right when errors occurred. This PR fixes that and adds interceptor-based tests to ensure things are working right. I still think we could use more tests but the PR is already quite large. ## Testing <!--- Please describe in detail how you tested your changes --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> This PR contains tests ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._
## Motivation and Context <!--- Why is this change required? What problem does it solve? --> <!--- If it fixes an open issue, please link to the issue here --> Necessary before we can implement retries. ## Description <!--- Describe your changes in detail --> I noticed that we weren't handling the flow quite right when errors occurred. This PR fixes that and adds interceptor-based tests to ensure things are working right. I still think we could use more tests but the PR is already quite large. ## Testing <!--- Please describe in detail how you tested your changes --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> This PR contains tests ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._
Motivation and Context
Necessary before we can implement retries.
Description
I noticed that we weren't handling the flow quite right when errors occurred. This PR fixes that and adds interceptor-based tests to ensure things are working right. I still think we could use more tests but the PR is already quite large.
Testing
This PR contains tests
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.