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

[BEAM-5759] Ensuring JmsIO checkpoint state is accessed and modified safely #6702

Merged
merged 3 commits into from
Oct 17, 2018

Conversation

andrew-flumaion
Copy link
Contributor

As described in BEAM-5759, a ConcurrentModificationException can be thrown when JmsIO source checkpoint finalisation occurs simultaneously to further messages being added for finalisation. This is due to the state of JmsCheckpointMark not being thread safe.

This change adds a unit test which demonstrates the issue by

  1. Adding several messages to a queue.
  2. Reading some of these messages with an UnboundedJmsReader (which adds messages for finalization).
  3. Calling finalizeCheckpoint on the reader's checkpoint mark in a separate thread.
  4. Reading the remainder of the messages in parallel with the above finalization.

In order to reliably reproduce this issue, the test uses a JMS consumer which decorates received messages with a callback which sleeps for a short period, introducing some "lag" into the finalization process.

To fix the issue, JmsCheckpointMark has been updated with thread safety constructs. In order to avoid potentially long blocks for consumers during checkpoint finalization, a read/write lock is used and finalization takes a snapshot of state before processing in a lock-free manner, only locking to update state post-finalization.

@jbonofre

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- --- --- --- ---
Java Build Status Build Status Build Status Build Status Build Status Build Status Build Status
Python Build Status --- Build Status
Build Status
Build Status --- --- ---

@andrew-flumaion
Copy link
Contributor Author

retest this please

@jbonofre jbonofre self-requested a review October 16, 2018 19:51
@jbonofre
Copy link
Member

retest this please

Copy link
Member

@jbonofre jbonofre left a comment

Choose a reason for hiding this comment

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

LGTM, thanks !

@jbonofre jbonofre merged commit d17d34d into apache:master Oct 17, 2018
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.

None yet

2 participants