Skip to content

Upgrade to AWS SDK v2 #58

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

Merged
merged 7 commits into from
Sep 27, 2021
Merged

Upgrade to AWS SDK v2 #58

merged 7 commits into from
Sep 27, 2021

Conversation

mngo87
Copy link
Contributor

@mngo87 mngo87 commented Sep 22, 2020

Issue #, if available:
#35

Description of changes:
Ensure this library is compatible with Java SDK V2

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

ArgumentCaptor<DeleteMessageBatchRequest> deleteBatchRequestCaptor = ArgumentCaptor.forClass(DeleteMessageBatchRequest.class);
verify(mockSqsBackend, times(1)).deleteMessageBatch(deleteBatchRequestCaptor.capture());
DeleteMessageBatchRequest request = deleteBatchRequestCaptor.getValue();
Assert.assertEquals(originalReceiptHandles.size(), request.entries().size());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i improved the test to check that the receipt handles were getting passed along correctly

SendMessageRequest.Builder sendMessageRequestBuilder = sendMessageRequest.toBuilder();
sendMessageRequestBuilder.overrideConfiguration(
AwsRequestOverrideConfiguration.builder()
.putHeader(USER_AGENT_HEADER_NAME, USER_AGENT_HEADER)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

please advise if this is not the best way to do this

@mngo87
Copy link
Contributor Author

mngo87 commented Sep 23, 2020

Since I am unable to attach reviewers. Just mentioning some folks. @robin-aws @adam-aws @aws-rizi

@prower-turnitin
Copy link

Hi, I took a look your branch for this pull request. I'm interested in having a version of this which works with AWS SDK 2 for Java. Your using the old payloadoffloading-common which use the old aws sdk v1.

There is a new version of this which works with v2.

        <dependency>
            <groupId>software.amazon.payloadoffloading</groupId>
            <artifactId>payloadoffloading-common</artifactId>
            <version>2.1.1</version>
        </dependency>

I may fork this repo myself to create a version which works with sdk 2. I'm fairly certain the above lib will be required.

@adam-aws
Copy link
Contributor

Hi @prower-turnitin , sorry for the late reply on this PR, yes we had previously refactored this library and moved some code out to payloadoffloading. We've added support to v2 in payloadoffloading so that we may eventually provide support for v2 for the extended client.
Yes you would need to use the payloadoffloading.

Thanks

@cruizba
Copy link

cruizba commented Mar 15, 2021

Hi @prower-turnitin , sorry for the late reply on this PR, yes we had previously refactored this library and moved some code out to payloadoffloading. We've added support to v2 in payloadoffloading so that we may eventually provide support for v2 for the extended client.
Yes you would need to use the payloadoffloading.

Thanks

That would be great!! Is there any ETA for the aws sdk v2 support?

Thanks in advance

@cemo
Copy link

cemo commented May 1, 2021

@adam-aws Would you please release new version of this library which depends on sdk2? We are ending up in our classpath mixed version of v1 and v2.

@@ -0,0 +1,58 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

done

@adam-aws adam-aws requested a review from eddy-aws September 2, 2021 17:42
@jordidomingo
Copy link

Hi! Do you have an ETA for v2 @eddy-aws ? We are also blocked with this release. Thanks

.queueUrl(SQS_QUEUE_URL)
.messageBody(messageBody)
.overrideConfiguration(
AwsRequestOverrideConfiguration.builder()
Copy link
Contributor

Choose a reason for hiding this comment

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

can we move this overrider to a helper method so that we can reuse it in other places?

AmazonS3 s3 = mock(AmazonS3.class);
when(s3.putObject(isA(PutObjectRequest.class))).thenReturn(null);
S3Client s3 = mock(S3Client.class);
// when(s3.putObject(isA(PutObjectRequest.class))).thenReturn(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

This line should be removed.

AmazonS3 s3 = mock(AmazonS3.class);
when(s3.putObject(isA(PutObjectRequest.class))).thenReturn(null);
S3Client s3 = mock(S3Client.class);
// when(s3.putObject(isA(PutObjectRequest.class))).thenReturn(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the comments

Comment on lines +600 to +601
// S3Object s3Object = S3Object.builder().build();
// s3Object.setObjectContent(new StringInputStream(expectedMessage));
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove

@juandavidg890121
Copy link

@mngo87 please review the comments we are blocked with this release

@eddy-aws
Copy link
Contributor

We can send a new PR for those comments. To unblock the customer, let's proceed and merge the PR.

@adam-aws adam-aws merged commit 129daf5 into awslabs:master Sep 27, 2021
@juandavidg890121
Copy link

juandavidg890121 commented Sep 27, 2021

We can send a new PR for those comments. To unblock the customer, let's proceed and merge the PR.

the branch has some problems with deprecated methods like withLargePayloadSupportEnabled or withPayloadSizeThreshold should be changed for withPayloadSupportEnabled and withPayloadSizeThreshold

method sendMessageBatch obtain the entries but never set to the sendMessageBatchRequest, something like the following should be done:
sendMessageBatchRequestBuilder.entries(batchEntries); sendMessageBatchRequest = sendMessageBatchRequestBuilder.build();

also the tests do not run due to dependency error
`java.lang.NoClassDefFoundError: com/fasterxml/jackson/databind/ObjectMapper

at software.amazon.payloadoffloading.JsonDataConverter.<init>(JsonDataConverter.java:19)
at software.amazon.payloadoffloading.PayloadS3Pointer.toJson(PayloadS3Pointer.java:35)`

all these changes I put in a local branch and tested

Note: versions of aws-java-sdk, junit and mockito could be increased

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.

8 participants