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

S3AsyncClient Support #19

Closed

Conversation

richardsmithsfdc
Copy link
Contributor

Issue #, if available: 17

Description of changes:

  • Bump library version
  • Move most configuration options to common base class, leaving S3Client
  • Add async configuration class, S3DAO, and PayloadOffloadingStore
  • Add unit tests

This change enables async SQS and S3 client support to be enabled in the Java SQS extended client. I have a companion PR for that repo if this one is accepted.

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

@scottgerring
Copy link
Contributor

@richardsmithsfdc was glad to discover you've already implemented this!

i've taken it and updated it to the current state of master here - #50

@kvcmc23
Copy link

kvcmc23 commented Feb 29, 2024

Does the support to AsyncClient has been released?

@richardsmithsfdc
Copy link
Contributor Author

Does the support to AsyncClient has been released?

It doesn't look like it. @scottgerring made a new PR syncing changes from the latest master at the end of last year, and there has been no movement on that either. I submitted my PR two years ago. It looks like we all have to continue to do what we've been doing, which is to put the async code in our own libraries.

@richardsmithsfdc
Copy link
Contributor Author

@richardsmithsfdc was glad to discover you've already implemented this!

i've taken it and updated it to the current state of master here - #50

@ziyanli-amazon , since you committed to this repo last year, is anyone there assigned to look at issues and features?

@scottgerring
Copy link
Contributor

scottgerring commented Mar 1, 2024

Let me see if I can try find someone responsible for this too.

@ziyanli-amazon
Copy link
Contributor

@richardsmithsfdc @scottgerring I will take a look next week after i get offcall.

Copy link
Contributor

@ziyanli-amazon ziyanli-amazon left a comment

Choose a reason for hiding this comment

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

Just saw this commit. You may disregard my previous comments regarding the naming of the method.

this.s3Async = s3Async;
this.s3BucketName = s3BucketName;
this.payloadSupport = true;
LOG.info("Payload support enabled.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we wanna add some text to distinguish standard and async s3 clients here?

* @param s3BucketName Name of the bucket which is going to be used for storing payload.
* The bucket must be already created and configured in s3.
*/
public void setPayloadSupportEnabled(S3AsyncClient s3Async, String s3BucketName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to have a different method name here?

@ziyanli-amazon
Copy link
Contributor

This PR is merged

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.

4 participants