Skip to content

Yet another add async support for S3#79

Merged
ziyanli-amazon merged 5 commits into
awslabs:masterfrom
richardsmithsfdc:master
Mar 6, 2024
Merged

Yet another add async support for S3#79
ziyanli-amazon merged 5 commits into
awslabs:masterfrom
richardsmithsfdc:master

Conversation

@richardsmithsfdc

@richardsmithsfdc richardsmithsfdc commented Oct 28, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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