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

Yet another add async support for S3 #79

Merged
merged 5 commits into from
Mar 6, 2024

Conversation

richardsmithsfdc
Copy link
Contributor

@richardsmithsfdc richardsmithsfdc commented Oct 28, 2022

Issue #, if available: 5

Description of changes:

Add async S3 support, based on latest async support in payload offloading. This PR cannot be merged until payload offloading version 2.2.0 with async support is released:

Creating this as a draft PR for now.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@ziyanli-amazon
Copy link
Contributor

@richardsmithsfdc Thanks for doing this! Appreciate all the amazing work done in this PR.

@ziyanli-amazon ziyanli-amazon marked this pull request as ready for review March 6, 2024 20:20
@ziyanli-amazon ziyanli-amazon merged commit fa60671 into awslabs:master Mar 6, 2024
@richardsmithsfdc
Copy link
Contributor Author

@ziyanli-amazon , thanks for merging this. FYI, I didn't do a ton of testing after updating to latest master. It was working before, and the UTs are still passing. I noticed that delete message is not chaining the S3 delete with the SQS delete. I think that should work because the result of the S3 delete doesn't really matter, but just wanted to let you know.

@richardsmithsfdc
Copy link
Contributor Author

@ziyanli-amazon , I remember the reason I wrote the code to ignore the S3 delete result, which is that if it is deleting multiple S3 messages, and one fails, I still wanted to delete the SQS messages because otherwise it might leave some messages in SQS that have their payloads deleted. This scenario can still occur with the synchronous extended client. In general, though, I think it's poor practice to leave unchained futures, just in case calling code wants to wait for all operations to complete. I will probably make a future PR to solve this issue in both.

Thanks again for merging the PRs!

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.

2 participants