-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[BEAM-7636] Migrate SqsIO to AWS SDK for Java 2 #9935
Conversation
29653f8
to
801d970
Compare
Thanks for contribution! |
Run Java PreCommit |
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.
Thanks! I did a review mostly over new added part and I expect that all other functionality/tests are left "as it was before".
Also, I think that @JohnRudolfLewis, as initial author of SqsIO, could be interested in review too.
...io/amazon-web-services2/src/main/java/org/apache/beam/sdk/io/aws2/sqs/SqsClientProvider.java
Outdated
Show resolved
Hide resolved
|
||
@ProcessElement | ||
public void processElement(ProcessContext processContext) throws Exception { | ||
sqs.sendMessage(processContext.element()); |
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.
Does it send message synchronically? Would it make sense to batch the messages of the whole bundle and send them as a batch for better performance?
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, that make send, but then our API has to take in SendMessageBatchRequestEntry, instead of SendMessageRequest. Now, there are three ways to deal with this:
- Take in SendMessageBatchRequestEntry, instead of SendMessageRequest. This way will impact all of the existing users
- Keep the existing api, which take in SendMessageRequest, and convert it to SendMessageBatchRequestEntry manually in our IO. Bascially, mapping each attribute of SendMessageRequest to SendMessageBatchRequest's
- Add another API, called
writeBatch
, and this one takes in SendMessageBatchRequestEntry, backward compatibility support
I prefer [BEAM-22] Support Unbounded PCollections in same-process execution #3, since it makes a clear distinction, users know what they are doing, and nobody get "hurt" ;-)
@aromanenko-dev let me know what you think?
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.
I'm ok to keep it as it was before for now. Perhaps, it would make sense to add TODO comment for the future about that.
sdks/java/io/amazon-web-services2/src/main/java/org/apache/beam/sdk/io/aws2/sqs/SqsIO.java
Outdated
Show resolved
Hide resolved
Please, take a look on failing tests in |
@cmachgodaddy Please, fix Spotless issue. |
@cmachgodaddy There are 2 failed "old" SqsIOTest tests. I think it's related to using the same host/port for |
Run CommunityMetrics PreCommit |
Run Python PreCommit |
1 similar comment
Run Python PreCommit |
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.
Thanks, LGTM
This PR is adding SqsIO v2 to amazon-web-services2 submodule, and which uses AWS SDK V2. There are a few changes comparing to SqsIO V1:
attributes
of theMessage
is not serializable, and to return attributes to downstream users. We will get an exceptionDefaultSdkContructionMap is not serializable
if we don't do this.Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username
).[BEAM-XXX] Fixes bug in ApproximateQuantiles
, where you replaceBEAM-XXX
with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.See the Contributor Guide for more tips on how to make review process smoother.
Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.