Io jms fix ack message checkpoint #22932

rvballada · 2022-08-29T14:17:01Z

Hi @lukecwik would you kindly review this PR related to the following issue:
#20814

Please find the following design documentation:
https://docs.google.com/document/d/19HiNPoJeIlzCFyWGdlw7WEFutceL2AmN_5Z0Vmi1s-I/edit?usp=sharing
assign to next reviewer:
R: @lukecwik

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

[ X] Choose reviewer(s) and mention them in a comment (R: @username).
[ X] Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
[ X] 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.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

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

github-actions · 2022-08-29T15:24:43Z

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @lukecwik for label java.
R: @chamikaramj for label io.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

github-actions · 2022-09-06T12:20:50Z

Reminder, please take a look at this pr: @lukecwik @chamikaramj

github-actions · 2022-09-09T12:14:04Z

Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment assign to next reviewer:

R: @apilloud for label java.
R: @johnjcasey for label io.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

github-actions · 2022-09-17T12:13:52Z

Reminder, please take a look at this pr: @apilloud @johnjcasey

sdks/java/io/jms/src/main/java/org/apache/beam/sdk/io/jms/JmsIO.java

github-actions · 2022-09-26T12:14:22Z

Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment assign to next reviewer:

R: @kileys for label java.
R: @pabloem for label io.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

sdks/java/io/jms/src/main/java/org/apache/beam/sdk/io/jms/JmsCheckpointMark.java

sdks/java/io/jms/src/main/java/org/apache/beam/sdk/io/jms/JmsIO.java

sdks/java/io/jms/src/main/java/org/apache/beam/sdk/io/jms/JmsCheckpointMark.java

- Upgrade equals and hashcode method in JmsCheckpointMark - Add a serciveExecutor.schedule method in order to close the JMS session after a tiemout and discard all the related checkpointd

sdks/java/io/jms/src/main/java/org/apache/beam/sdk/io/jms/JmsCheckpointMark.java

sdks/java/io/jms/src/main/java/org/apache/beam/sdk/io/jms/JmsIO.java

sdks/java/io/jms/src/main/java/org/apache/beam/sdk/io/jms/JmsCheckpointMark.java

sdks/java/io/jms/src/main/java/org/apache/beam/sdk/io/jms/JmsIO.java

- Get back to the initial implementation of JmsCheckpointMark - Add the discard attribute and discard() method to JmsCheckpointMark

lukecwik · 2022-10-13T20:32:14Z

Sorry for the long wait, lots of other issues blocked me from reviewing this. Taking a follow-up look now.

lukecwik

All my comments are minor other then improving how we author the testCloseWithTimeout so it isn't reliant on Thread.sleep

sdks/java/io/jms/src/main/java/org/apache/beam/sdk/io/jms/JmsCheckpointMark.java

sdks/java/io/jms/src/main/java/org/apache/beam/sdk/io/jms/JmsIO.java

sdks/java/io/jms/src/main/java/org/apache/beam/sdk/io/jms/JmsCheckpointMark.java

sdks/java/io/jms/src/test/java/org/apache/beam/sdk/io/jms/JmsIOTest.java

lukecwik · 2022-10-13T20:57:56Z

sdks/java/io/jms/src/test/java/org/apache/beam/sdk/io/jms/JmsIOTest.java

+    JmsIO.UnboundedJmsReader reader = source.createReader(null, null);
+
+    reader.start();
+    reader.close();


I would suggest using a mock ScheduledExecutorService that you set on the PipelineOptions object when creating the reader. This way you can inject here in the test and capture the runnable/callable directly without needing to have a test reliant on Thread.sleep

How do we get the executor service from ExecutorOptions? As far as I can see, PipelineOptions should be then a reader field, but how do we get the ExecutorService from PipelineOptions?

pipelineOptions.as(ExecutorOptions.class).getScheduledExecutorService(), the key part being the as(...) method

Yes I get it. I don't know exactly how to mock it.

sdks/java/io/jms/src/test/java/org/apache/beam/sdk/io/jms/JmsIOTest.java

…nalizeCheckpoint method Co-authored-by: Lukasz Cwik <[email protected]>

… discarded

lukecwik · 2022-10-15T00:33:42Z

#23234 was merged allowing you to use the ScheduledExecutorService from PipelineOptions

sdks/java/io/jms/src/test/java/org/apache/beam/sdk/io/jms/JmsIOTest.java

lukecwik · 2022-10-17T16:27:29Z

sdks/java/io/jms/src/main/java/org/apache/beam/sdk/io/jms/JmsIO.java

-          consumer = null;
+        closeAutoscaler();
+        closeConsumer();
+        ScheduledExecutorService executorService = Executors.newSingleThreadScheduledExecutor();


pipelineOptions.as(ExecutorOptions.class).getScheduledExecutorService(), the key part being the as(...) method

lukecwik · 2022-10-17T17:47:34Z

Note that the next release cut is this Wednesday so if your able to clean-up this PR we could merge it and it would make its way into the 2.43 release otherwise it will be another 6 weeks before the next release cut.

Co-authored-by: Lukasz Cwik <[email protected]>

…orService

… (don't know if it is better)

lukecwik

I'll try updating the test to use a mock.

sdks/java/io/jms/src/main/java/org/apache/beam/sdk/io/jms/JmsIO.java

sdks/java/io/jms/src/test/java/org/apache/beam/sdk/io/jms/JmsIOTest.java

lukecwik · 2022-10-18T20:28:35Z

Updated to use a mock, now waiting for results of test run before merging.

sdks/java/io/jms/src/test/java/org/apache/beam/sdk/io/jms/JmsIOTest.java

…OTest.java

lukecwik · 2022-10-18T22:33:23Z

Run Java PreCommit

rvballada · 2022-10-19T08:56:28Z

Thanks @lukecwik !

rvballada added 7 commits April 26, 2022 10:49

[BEAM-11828] => Fix read message queue implementation

81257d6

Merge branch 'apache:master' into io_jms_fix_ack_message_checkpoint

6c8cdbb

[BEAM-11828] => New implementation to fix acknowledgment

d5d9bcd

[BEAM-11828] => Some refactoring (remove drainMessage method)

c5ab8ef

Merge branch 'apache:master' into io_jms_fix_ack_message_checkpoint

e93c31a

Merge branch 'apache:master' into io_jms_fix_ack_message_checkpoint

3d3ce77

Update CHANGES.md

d16d154

github-actions bot added io java jms labels Aug 29, 2022

github-actions bot added the Next Action: Reviewers label Aug 29, 2022

github-actions bot added the slow-review label Sep 6, 2022

github-actions bot removed the slow-review label Sep 9, 2022

github-actions bot added the slow-review label Sep 17, 2022

johnjcasey requested changes Sep 19, 2022

View reviewed changes

sdks/java/io/jms/src/main/java/org/apache/beam/sdk/io/jms/JmsIO.java Outdated Show resolved Hide resolved

github-actions bot removed the slow-review label Sep 26, 2022

lukecwik reviewed Sep 26, 2022

View reviewed changes

Pull request review:

5b14193

- Upgrade equals and hashcode method in JmsCheckpointMark - Add a serciveExecutor.schedule method in order to close the JMS session after a tiemout and discard all the related checkpointd

lukecwik reviewed Sep 29, 2022

View reviewed changes

rvballada added 2 commits September 30, 2022 18:48

Pull request review:

0a83c78

- Get back to the initial implementation of JmsCheckpointMark - Add the discard attribute and discard() method to JmsCheckpointMark

Adding some unit tests to new JmsCHeckpointMark discard method

8158363

lukecwik requested changes Oct 13, 2022

View reviewed changes

rvballada and others added 2 commits October 14, 2022 09:18

Code review: discard checkpoint and clear messages at beginning of fi…

490b849

…nalizeCheckpoint method Co-authored-by: Lukasz Cwik <[email protected]>

Throw an IllegalStateException when adding message when checkpoint is…

b16e9f6

… discarded

rvballada added 2 commits October 14, 2022 09:57

Change closeTimeout from long to Duration

4b1d6ad

CHeck that closeTimeout is non negative

aa24d45

rvballada added 2 commits October 17, 2022 08:57

Merge branch 'apache:master' into io_jms_fix_ack_message_checkpoint

5e3c4be

Add private package fields to perform testd

d91bc23

lukecwik reviewed Oct 17, 2022

View reviewed changes

rvballada and others added 5 commits October 18, 2022 08:47

Code review: update comment

803b964

Co-authored-by: Lukasz Cwik <[email protected]>

Code review: add comment

602cb2c

Co-authored-by: Lukasz Cwik <[email protected]>

Code review: remove empty space

b7bb897

Co-authored-by: Lukasz Cwik <[email protected]>

Code review: use ExecutorOptions to get an instance of ShceduleExecut…

497b191

…orService

Code review: avoid Thread.sleep with ExecutorService.awaitTermination…

c3888bb

… (don't know if it is better)

lukecwik reviewed Oct 18, 2022

View reviewed changes

Apply suggestions from code review

c5dd19f

lukecwik approved these changes Oct 18, 2022

View reviewed changes

Apply suggestions from code review

0a8c633

lukecwik approved these changes Oct 18, 2022

View reviewed changes

sdks/java/io/jms/src/test/java/org/apache/beam/sdk/io/jms/JmsIOTest.java Outdated Show resolved Hide resolved

sdks/java/io/jms/src/test/java/org/apache/beam/sdk/io/jms/JmsIOTest.java Outdated Show resolved Hide resolved

Apply suggestions from code review

a8df80b

lukecwik approved these changes Oct 18, 2022

View reviewed changes

sdks/java/io/jms/src/test/java/org/apache/beam/sdk/io/jms/JmsIOTest.java Outdated Show resolved Hide resolved

Update sdks/java/io/jms/src/test/java/org/apache/beam/sdk/io/jms/JmsI…

2611331

…OTest.java

lukecwik merged commit 10977bf into apache:master Oct 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Io jms fix ack message checkpoint #22932

Io jms fix ack message checkpoint #22932

rvballada commented Aug 29, 2022 •

edited

Loading

github-actions bot commented Aug 29, 2022

github-actions bot commented Sep 6, 2022

github-actions bot commented Sep 9, 2022

github-actions bot commented Sep 17, 2022

github-actions bot commented Sep 26, 2022

lukecwik commented Oct 13, 2022

lukecwik left a comment

lukecwik Oct 13, 2022

rvballada Oct 17, 2022

lukecwik Oct 17, 2022

rvballada Oct 18, 2022

lukecwik commented Oct 15, 2022

lukecwik Oct 17, 2022

lukecwik commented Oct 17, 2022

lukecwik left a comment

lukecwik commented Oct 18, 2022

lukecwik commented Oct 18, 2022

rvballada commented Oct 19, 2022

Io jms fix ack message checkpoint #22932

Io jms fix ack message checkpoint #22932

Conversation

rvballada commented Aug 29, 2022 • edited Loading

GitHub Actions Tests Status (on master branch)

github-actions bot commented Aug 29, 2022

github-actions bot commented Sep 6, 2022

github-actions bot commented Sep 9, 2022

github-actions bot commented Sep 17, 2022

github-actions bot commented Sep 26, 2022

lukecwik commented Oct 13, 2022

lukecwik left a comment

Choose a reason for hiding this comment

lukecwik Oct 13, 2022

Choose a reason for hiding this comment

rvballada Oct 17, 2022

Choose a reason for hiding this comment

lukecwik Oct 17, 2022

Choose a reason for hiding this comment

rvballada Oct 18, 2022

Choose a reason for hiding this comment

lukecwik commented Oct 15, 2022

lukecwik Oct 17, 2022

Choose a reason for hiding this comment

lukecwik commented Oct 17, 2022

lukecwik left a comment

Choose a reason for hiding this comment

lukecwik commented Oct 18, 2022

lukecwik commented Oct 18, 2022

rvballada commented Oct 19, 2022

rvballada commented Aug 29, 2022 •

edited

Loading