Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
## New Features / Improvements

* (Python) Removed the `envoy-data-plane` (and transitive `betterproto`) dependency; `EnvoyRateLimiter` now uses a small vendored protobuf definition instead, resolving dependency conflicts for downstream projects ([#37854](https://github.com/apache/beam/issues/37854)).
* (Java) Supported acknowledge mode for JmsIO ([#39253](https://github.com/apache/beam/issues/39253)).
* X feature added (Java/Python) ([#X](https://github.com/apache/beam/issues/X)).

## Breaking Changes
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,16 +173,17 @@ public void processElement(
} else {
Instant watermark = reader.getWatermark();
if (watermark.isBefore(BoundedWindow.TIMESTAMP_MAX_VALUE)) {
// If the reader had no elements available, but the shard is not done, reuse it later
// Might be better to finalize old checkpoint.
// If the reader had no elements available, but the shard is not done, reuse it later.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

in a typical run, 5/6 tests observe small amount of messages are not acked due to the known issue noted here:

// TODO(yathu) resolve pending messages with direct runner then we can simply assert

  • testPublishingThenReadingAllClientAcknowledgeUnsafe: 1
  • testPublishingThenReadingAllIndividualAcknowledge: 13
  • testPublishingThenReadingAllIndividualAcknowledge: 13
  • testPublishingThenReadingAll: 34
  • testPublishingThenReadingAll: 33

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

How about on dataflow runner? Do we have the same problem?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will follow up on Dataflow runner (need a remote ActiveMQ deployement)

The original report where this issue was observed was on Direct runner: #30218 (comment)

// Finalize old checkpoint now.
final CheckpointMarkT checkpoint = shard.getCheckpoint();
if (checkpoint != null) {
checkpoint.finalizeCheckpoint();
}
resultBuilder.addUnprocessedElements(
Collections.<WindowedValue<?>>singleton(
WindowedValues.timestampedValueInGlobalWindow(
UnboundedSourceShard.of(
shard.getSource(),
shard.getDeduplicator(),
reader,
shard.getCheckpoint()),
shard.getSource(), shard.getDeduplicator(), reader, null),
watermark)));
} else {
// End of input. Close the reader after finalizing old checkpoint.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,17 @@

import java.io.IOException;
import java.io.Serializable;
import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import javax.jms.JMSException;
import javax.jms.Message;
import javax.jms.MessageConsumer;
import javax.jms.Session;
import org.apache.beam.sdk.io.UnboundedSource;
import org.apache.beam.sdk.io.jms.JmsIO.AcknowledgeMode;
import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.annotations.VisibleForTesting;
import org.checkerframework.checker.nullness.qual.Nullable;
import org.joda.time.Instant;
Expand All @@ -41,29 +45,32 @@ class JmsCheckpointMark implements UnboundedSource.CheckpointMark, Serializable
private static final Logger LOG = LoggerFactory.getLogger(JmsCheckpointMark.class);

private Instant oldestMessageTimestamp;
private transient @Nullable Message lastMessage;
private transient @Nullable List<Message> messages;
private transient @Nullable MessageConsumer consumer;
private transient @Nullable Session session;
private transient @Nullable AtomicInteger activeCheckpoints;

private JmsCheckpointMark(
Instant oldestMessageTimestamp,
@Nullable Message lastMessage,
@Nullable List<Message> messages,
@Nullable MessageConsumer consumer,
@Nullable Session session) {
@Nullable Session session,
@Nullable AtomicInteger activeCheckpoints) {
this.oldestMessageTimestamp = oldestMessageTimestamp;
this.lastMessage = lastMessage;
this.messages = messages;
this.consumer = consumer;
this.session = session;
this.activeCheckpoints = activeCheckpoints;
}

/** Acknowledge all outstanding message. */
@Override
public void finalizeCheckpoint() {
try {
// Jms spec will implicitly acknowledge _all_ messaged already received by the same
// session if one message in this session is being acknowledged.
if (lastMessage != null) {
lastMessage.acknowledge();
if (messages != null) {
for (Message message : messages) {
message.acknowledge();
Comment on lines +71 to +72

@tkaymak tkaymak Jul 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Question on the two new modes when finalization runs on a separate thread:
In INDIVIDUAL_ACKNOWLEDGE and CLIENT_ACKNOWLEDGE_UNSAFE the mark holds Messages bound to the reader's single long lived session. My understanding is that finalizeCheckpoint() may be called on a different thread from the reader on some runners.
You would know the Dataflow specifics far better than I do. If so, does message.acknowledge() here end up running concurrently with the reader's receive loop on the same Session, which JMS (section 4.4.6) says must have a single thread of control? CLIENT_ACKNOWLEDGE looks immune since each mark owns a private session. Could you confirm how ack is serialized against receive for the other two modes, or which providers this has been validated against? (This ties into @shunping 's Dataflow question above).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that finalizeCheckpoint() may be called on a different thread from the reader on some runners.

Correct

does message.acknowledge() here running concurrently with the reader's receive loop on the same Session

No for INDIVIDUAL_ACKNOWLEDGE and CLIENT_ACKNOWLEDGE_UNSAFE. That's why the latter is "unsafe". The former isn't part of Jms spec but some providers' extended feature.

Could you confirm how ack is serialized against receive for the other two modes, or which providers this has been validated against?

The integration tests now covers this question, tested on ActiveMQ and Amqp provider.

CLIENT_ACKNOWLEDGE looks immune since each mark owns a private session

Yes

@tkaymak tkaymak Jul 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you, that mostly clears it up! One part I still want to nail/understand:

The CheckpointMark contract in UnboundedSource.java says finalizeCheckpoint "may be called from any thread, concurrently with calls to the UnboundedReader it was created from", and the reader is single threaded but finalize is exempted. For INDIVIDUAL_ACKNOWLEDGE and CLIENT_ACKNOWLEDGE_UNSAFE getCheckpointMark does not recreate the session, so the mark's messages stay bound to the reader's live session, and finalizeCheckpoint calls message.acknowledge() on it without taking the reader monitor. CLIENT_ACKNOWLEDGE avoids this because it hands each checkpoint its own session.

On Dataflow, where commit finalization runs asynchronously on a pool thread while the cached reader keeps advancing, what stops acknowledge() from racing the reader's receiveNoWait() on the same session? Same question for the legacy Flink UnboundedSourceWrapper, which calls finalizeCheckpoint outside the checkpoint lock. The integration tests run on the direct runner, where finalize is on the reader thread, so they would not exercise this. (Or am I missing something?)

If I am missing the mechanism that serializes them I am happy to be corrected. Otherwise acking under the reader monitor for these two modes, or giving them per checkpoint sessions like CLIENT_ACKNOWLEDGE, would close this.

}
}
} catch (JMSException e) {
// The effect of this is message not get acknowledged and thus will be redelivered. It is
Expand Down Expand Up @@ -93,14 +100,37 @@ public void finalizeCheckpoint() {
LOG.info("Error closing JMS session. It may have already been closed.");
}
}

if (activeCheckpoints != null) {
activeCheckpoints.decrementAndGet();
}
Comment thread
Abacn marked this conversation as resolved.
}

@VisibleForTesting
@Nullable
List<Message> getMessages() {
return messages;
}

@VisibleForTesting
@Nullable
Session getSession() {
return session;
}

@VisibleForTesting
@Nullable
MessageConsumer getConsumer() {
return consumer;
}

// set an empty list to messages when deserialize
private void readObject(java.io.ObjectInputStream stream)
throws IOException, ClassNotFoundException {
stream.defaultReadObject();
lastMessage = null;
messages = null;
session = null;
consumer = null;
}

@Override
Expand All @@ -120,24 +150,27 @@ public int hashCode() {
return Objects.hash(oldestMessageTimestamp);
}

static Preparer newPreparer() {
return new Preparer();
static Preparer newPreparer(AcknowledgeMode acknowledgeMode) {
return new Preparer(acknowledgeMode);
}

/**
* A class preparing the immutable checkpoint. It is mutable so that new messages can be added.
*/
static class Preparer {
private Instant oldestMessageTimestamp = Instant.now();
private transient @Nullable Message lastMessage = null;
private transient List<Message> messages = new ArrayList<>();
private final AcknowledgeMode acknowledgeMode;

@VisibleForTesting transient boolean discarded = false;

@VisibleForTesting final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();

private Preparer() {}
private Preparer(AcknowledgeMode acknowledgeMode) {
this.acknowledgeMode = acknowledgeMode;
}

void add(Message message) throws Exception {
void add(Message message) throws JMSException {
lock.writeLock().lock();
try {
if (discarded) {
Expand All @@ -149,7 +182,18 @@ void add(Message message) throws Exception {
if (currentMessageTimestamp.isBefore(oldestMessageTimestamp)) {
oldestMessageTimestamp = currentMessageTimestamp;
}
lastMessage = message;
if (acknowledgeMode == AcknowledgeMode.INDIVIDUAL_ACKNOWLEDGE) {
messages.add(message);
} else {
// Jms spec will implicitly acknowledge _all_ messaged already received by the same
// session if one message in this session is being acknowledged. Only need to ack
// last seen one.
if (messages.isEmpty()) {
messages.add(message);
} else {
messages.set(0, message);
}
}
} finally {
lock.writeLock().unlock();
}
Expand All @@ -167,6 +211,7 @@ Instant getOldestMessageTimestamp() {
void discard() {
lock.writeLock().lock();
try {
messages.clear();
this.discarded = true;
} finally {
lock.writeLock().unlock();
Expand All @@ -175,21 +220,43 @@ void discard() {

/**
* Create a new checkpoint mark based on the current preparer. This will reset the messages held
* by the preparer, and the owner of the preparer is responsible to create a new Jms session
* after this call.
* by the preparer. If AcknowledgeMode is CLIENT_ACKNOWLEDGE, the owner of the preparer is
* responsible to create a new Jms session after this call.
*/
JmsCheckpointMark newCheckpoint(@Nullable MessageConsumer consumer, @Nullable Session session) {
JmsCheckpointMark newCheckpoint(
@Nullable MessageConsumer consumer,
@Nullable Session session,
@Nullable AcknowledgeMode acknowledgeMode,
@Nullable AtomicInteger activeCheckpoints) {
JmsCheckpointMark checkpointMark;
lock.writeLock().lock();
try {
if (discarded) {
lastMessage = null;
messages.clear();
checkpointMark = this.emptyCheckpoint();
} else {
List<Message> messagesCopy = null;
MessageConsumer consumerToPass = null;
Session sessionToPass = null;
if (!messages.isEmpty()) {
messagesCopy = new ArrayList<>(messages);
}
if (acknowledgeMode == AcknowledgeMode.CLIENT_ACKNOWLEDGE) {
consumerToPass = consumer;
sessionToPass = session;
}
checkpointMark =
new JmsCheckpointMark(oldestMessageTimestamp, lastMessage, consumer, session);
lastMessage = null;
new JmsCheckpointMark(
oldestMessageTimestamp,
messagesCopy,
consumerToPass,
sessionToPass,
activeCheckpoints);
messages.clear();
oldestMessageTimestamp = Instant.now();
if (activeCheckpoints != null) {
activeCheckpoints.incrementAndGet();
}
}
} finally {
lock.writeLock().unlock();
Expand All @@ -198,11 +265,11 @@ JmsCheckpointMark newCheckpoint(@Nullable MessageConsumer consumer, @Nullable Se
}

JmsCheckpointMark emptyCheckpoint() {
return new JmsCheckpointMark(oldestMessageTimestamp, null, null, null);
return new JmsCheckpointMark(oldestMessageTimestamp, null, null, null, null);
}

boolean isEmpty() {
return lastMessage == null;
return messages.isEmpty();
}
}
}
Loading
Loading