[BEAM-11907] SQS source with full functionality by dennisylyung · Pull Request #14244 · apache/beam

dennisylyung · 2021-03-16T02:38:16Z

The current SqsIO source connector has limited functionality.
This PR implements the following functionalities to SqsIO:

Batch message deletion (mitigate throttled deleteMessage requests)
Extension of visibility timeout (The existing implementation causes message duplication)
Update watermarks when no new messages are pulled
New functionalities are written with reference to the pubsub connector.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang	Dataflow	Samza	Twister2
Go	---	---	---
Java
Python		---	---
XLang		---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website	Whitespace	Typescript
Non-portable
Portable	---		---	---	---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

codecov · 2021-03-16T02:56:23Z

Codecov Report

Merging #14244 (5c2264f) into master (6cdf356) will decrease coverage by 0.80%.
The diff coverage is n/a.

❗ Current head 5c2264f differs from pull request most recent head d0463a7. Consider uploading reports for the commit d0463a7 to get more accurate results

@@            Coverage Diff             @@
##           master   #14244      +/-   ##
==========================================
- Coverage   83.66%   82.86%   -0.81%     
==========================================
  Files         446      466      +20     
  Lines       59239    57587    -1652     
==========================================
- Hits        49564    47720    -1844     
- Misses       9675     9867     +192

Impacted Files	Coverage Δ
..._beam/testing/benchmarks/nexmark/queries/query2.py
...eam/runners/interactive/interactive_environment.py
...ild/srcs/sdks/python/apache_beam/utils/counters.py
...n/apache_beam/runners/dataflow/dataflow_metrics.py
...build/srcs/sdks/python/apache_beam/utils/plugin.py
...uild/srcs/sdks/python/apache_beam/io/gcp/pubsub.py
...on/apache_beam/runners/worker/statesampler_slow.py
..._beam/testing/benchmarks/nexmark/queries/query5.py
...s/python/apache_beam/examples/snippets/snippets.py
...s/sdks/python/apache_beam/examples/avro_bitcoin.py
... and 902 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6cdf356...d0463a7. Read the comment docs.

boyuanzz · 2021-03-16T17:30:10Z

cc: @boyuanzz

aaltay · 2021-04-01T17:57:07Z

@dennisylyung - Thank you for your contribution. Is this PR ready for review?

dennisylyung · 2021-04-04T23:31:20Z

@dennisylyung - Thank you for your contribution. Is this PR ready for review?

Not yet sorry.
I am implementing for the aws2 module too.

dennisylyung · 2021-04-09T01:15:03Z

Run Java PreCommit

dennisylyung · 2021-04-09T05:55:52Z

Run Java PreCommit

dennisylyung · 2021-04-12T02:07:50Z

R: @aaltay
It is ready for review now, thanks!
The PR turned out to be more rewriting the connectors referencing the pubsub one than doing small enhancements, since when I was developing a job with the sqs reader I found that it lacks many functionalities to make it generally usable.
Please let me know how to properly document such changes.

aaltay · 2021-04-20T02:40:24Z

@dennisylyung - Thank you!

Not sure who might be a good reviewer for this PR. Do you mind pinging the dev@ thread you started (or a new thread) ask for reviews?

dennisylyung · 2021-04-20T03:31:22Z

@iemejia Do you think you can review, or ping someone who is suitable? Thanks!

iemejia · 2021-04-20T10:29:54Z

@dennisylyung I am a bit busy this week but maybe @aromanenko-dev can take a look. In the main time can you please rebase and squash your commits. Thanks !

dennisylyung · 2021-04-21T03:30:21Z

Run Java PreCommit

dennisylyung · 2021-04-21T09:01:31Z

Run Java PreCommit

aromanenko-dev · 2021-04-21T16:25:29Z

@iemejia Sure, I'll take a look as soon as I can (I hope in a couple of next days).

aromanenko-dev · 2021-04-21T16:28:49Z

In the same time, I see that these 2 gradle tasks failed:
:sdks:java:io:amazon-web-services:compileJava
:sdks:java:io:amazon-web-services2:compileJava

More details are here

aromanenko-dev · 2021-04-22T07:08:26Z

Run Java PreCommit

dennisylyung · 2021-04-23T05:21:02Z

Run Java PreCommit

dennisylyung · 2021-04-23T05:24:47Z

Run Java_Examples_Dataflow_Java11 PreCommit

checkstyle errors spotless restore nullness check suppression spotless supress nullness warning in checkpointmark''

dennisylyung · 2021-04-23T07:07:51Z

@aromanenko-dev It passes the java precommit tests now

aromanenko-dev · 2021-04-23T15:22:21Z

@dennisylyung Thanks! I'll take a look in the next week.

aaltay · 2021-05-06T22:54:17Z

@dennisylyung Thanks! I'll take a look in the next week.

@aromanenko-dev, thank you and a gentle ping to get this in your review queue.

aromanenko-dev · 2021-05-10T12:21:14Z

Sorry, I was quite busy last week. I'll try to take a look asap.

aromanenko-dev

Code looks good for me in general, just a couple of notes. Though, since it's mostly based on Pubsub implementation (iiic), then it would make sense to ask someone, who knows PubsubIO well, take a look on this. Wdyt, @aaltay ?

.../io/amazon-web-services/src/main/java/org/apache/beam/sdk/io/aws/sqs/SqsUnboundedReader.java

aromanenko-dev · 2021-05-10T12:38:08Z

sdks/java/io/amazon-web-services/src/test/java/org/apache/beam/sdk/io/aws/sqs/SqsIOTest.java

+  }
+
+  @Test
+  public void checkpointCoderIsSane() {


Please, add "test" prefix for all test methods

changed in the new commit

aromanenko-dev

It looks fine for me in general (since new functionality is mostly based on PubsubReader ones). I have just a minor request about tests.

@aaltay Please, ping someone who knows this PubsubReader functionality and who can review this in more details.

aromanenko-dev · 2021-05-24T12:29:36Z

sdks/java/io/amazon-web-services/src/test/java/org/apache/beam/sdk/io/aws/sqs/SqsIOTest.java

-    final PCollection<Message> output =
-        pipeline.apply(SqsIO.read().withQueueUrl(queueUrl).withMaxNumRecords(100));
+  @Test
+  public void testReadOneMessage() throws IOException {


Please, move all Reader's tests to a dedicated test class. e.g. SqsUnboundedReaderTest

changed in the new commit

aromanenko-dev · 2021-05-24T12:32:26Z

sdks/java/io/amazon-web-services2/src/test/java/org/apache/beam/sdk/io/aws2/sqs/SqsIOTest.java

+  }

+  @Test
+  public void testReadOneMessage() throws IOException {


Please, move all Reader's tests to a dedicated test class. e.g. SqsUnboundedReaderTest

changed in the new commit

aaltay · 2021-05-24T19:03:01Z

@dpcollins-google / @boyuanzz - Would one of you also review this change? (@aromanenko-dev mentioned that most of the change is based on the pubsub io implementation.)

dpcollins-google · 2021-05-24T19:04:22Z

@boyuanzz is probably a better reviewer, but I can if they're too busy. Reach out to me internally if you need my review

aaltay · 2021-06-11T01:32:56Z

@boyuanzz - Would you be able to review this when you get a chance?

dennisylyung · 2021-06-18T05:12:59Z

Run Java PreCommit

dennisylyung · 2021-06-18T06:04:29Z

Run Java PreCommit

dennisylyung · 2021-06-18T08:34:10Z

@aromanenko-dev I have separated the tests as suggested in your last review.
However, the java tests seem to fail on other modules. Any idea why?

aromanenko-dev · 2021-06-18T11:30:27Z

@dennisylyung I don't think it's related to your change since only org.apache.beam.sdk.io.elasticsearch.ElasticsearchIOTest.testWriteScriptedUpsert fails.
Though, it can be related to this discussion.

aromanenko-dev · 2021-06-18T11:31:05Z

Run Java PreCommit

aromanenko-dev

If there are no principal objections then I think it can be merged.

aaltay · 2021-07-01T21:30:07Z

If there are no principal objections then I think it can be merged.

I agree. There is one approval, and all tests pass. Feel free to merge.

dennisylyung force-pushed the issue/BEAM-11907 branch from 5c2264f to 0f88155 Compare March 16, 2021 02:52

dennisylyung force-pushed the issue/BEAM-11907 branch from 0f88155 to 3e8160b Compare March 23, 2021 06:14

dennisylyung marked this pull request as ready for review April 12, 2021 02:04

iemejia requested a review from aromanenko-dev April 20, 2021 10:30

dennisylyung force-pushed the issue/BEAM-11907 branch from a31475f to 3948f03 Compare April 21, 2021 02:49

dennisylyung force-pushed the issue/BEAM-11907 branch from c3fa627 to d4a4d4b Compare April 22, 2021 02:00

dennisylyung force-pushed the issue/BEAM-11907 branch from 03d3af9 to 231518f Compare April 22, 2021 08:58

[BEAM-11907] Full function SqsIO Consumer''

ada287a

checkstyle errors spotless restore nullness check suppression spotless supress nullness warning in checkpointmark''

dennisylyung force-pushed the issue/BEAM-11907 branch from 71711ec to ada287a Compare April 23, 2021 06:18

aromanenko-dev reviewed May 11, 2021

View reviewed changes

Add "test" prefix to tests

52d7400

aromanenko-dev requested changes May 24, 2021

View reviewed changes

aaltay requested a review from boyuanzz June 3, 2021 18:52

Separate tests to SqsUnboundedReaderTest and SqsUnboundedSourceTest

d0463a7

aromanenko-dev approved these changes Jun 23, 2021

View reviewed changes

aromanenko-dev merged commit 1c9fff2 into apache:master Jul 5, 2021

Conversation

dennisylyung commented Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

GitHub Actions Tests Status (on master branch)

Uh oh!

codecov bot commented Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

boyuanzz commented Mar 16, 2021

Uh oh!

aaltay commented Apr 1, 2021

Uh oh!

dennisylyung commented Apr 4, 2021

Uh oh!

dennisylyung commented Apr 9, 2021

Uh oh!

dennisylyung commented Apr 9, 2021

Uh oh!

dennisylyung commented Apr 12, 2021

Uh oh!

aaltay commented Apr 20, 2021

Uh oh!

dennisylyung commented Apr 20, 2021

Uh oh!

iemejia commented Apr 20, 2021

Uh oh!

dennisylyung commented Apr 21, 2021

Uh oh!

dennisylyung commented Apr 21, 2021

Uh oh!

aromanenko-dev commented Apr 21, 2021

Uh oh!

aromanenko-dev commented Apr 21, 2021

Uh oh!

aromanenko-dev commented Apr 22, 2021

Uh oh!

dennisylyung commented Apr 23, 2021

Uh oh!

dennisylyung commented Apr 23, 2021

Uh oh!

dennisylyung commented Apr 23, 2021

Uh oh!

aromanenko-dev commented Apr 23, 2021

Uh oh!

aaltay commented May 6, 2021

Uh oh!

aromanenko-dev commented May 10, 2021

Uh oh!

aromanenko-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aromanenko-dev May 10, 2021

Choose a reason for hiding this comment

Uh oh!

dennisylyung May 18, 2021

Choose a reason for hiding this comment

Uh oh!

aromanenko-dev left a comment

Choose a reason for hiding this comment

Uh oh!

aromanenko-dev May 24, 2021

Choose a reason for hiding this comment

Uh oh!

dennisylyung Jun 18, 2021

Choose a reason for hiding this comment

Uh oh!

aromanenko-dev May 24, 2021

Choose a reason for hiding this comment

Uh oh!

dennisylyung Jun 18, 2021

Choose a reason for hiding this comment

Uh oh!

aaltay commented May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dpcollins-google commented May 24, 2021

dennisylyung commented Mar 16, 2021 •

edited

Loading

codecov bot commented Mar 16, 2021 •

edited

Loading

aaltay commented May 24, 2021 •

edited

Loading