Kafka Connect: Harden commit coordinator (fencing, recovery, dedup, retryable commits)

[kumarpritam863]

What

Hardens the Iceberg Kafka Connect sink's commit Coordinator — its zombie-fencing,
recovery, control-topic consumption, and shutdown paths.

Coordinator hardening

• Zombie fencing via a fixed transactional.id. The coordinator producer id is now
  <connectGroupId>-<connectorName>-coord — identical across a connector's tasks and stable
  across restarts/reconfigurations — so a newly elected coordinator's initTransactions()
  epoch-bumps and fences a prior (zombie) coordinator's control-plane writes. Worker ids are
  unchanged (<prefix>worker<suffix>), so sibling workers never fence each other. Solves Metadata stop to be written after random period of time. Commit failed, will try again next cycle. #13593 (comment)

• Fenced ≠ fatal. A coordinator that terminates because it was fenced
  (ProducerFencedException / InvalidProducerEpochException / UnknownProducerIdException,
  matched across the cause chain up to a bounded depth) is cleared without failing the task
  (Connect does not auto-restart FAILED tasks); any other coordinator termination still fails
  the task loudly.

• Recovery reads from earliest. The coordinator's -coord consumer group now defaults
  to auto.offset.reset=earliest, so a fresh or expired group re-reads uncommitted control
  events instead of skipping to the log end and dropping their data. Replay is idempotent via
  the snapshot offset floor + distinctByKey(location) dedup + the offsets compare-and-swap.

• Idempotency filter in Channel.consumeAvailable. Control-topic records at or below the
  already-consumed per-partition offset are skipped, so a re-delivered or rewound record is
  never re-buffered or re-counted. The per-partition offset is only ever advanced. This solves #16282

• Transient Kafka commit errors are retryable. A consumer-offset commit hiccup from
  commitConsumerOffsets() (Iceberg CommitFailedException, Kafka CommitFailedException,
  RebalanceInProgressException, RetriableException — e.g. the -coord consumer briefly
  evicted when a catalog commit exceeds max.poll.interval.ms) no longer fails the task: the
  table commit already succeeded and the watermark re-advances on the next cycle. Non-retryable
  errors still terminate the coordinator.

• Bounded, interrupt-safe shutdown. stopCoordinator clears state first, then signals
  termination and waits (bounded, 60s) for the thread to release its producer/consumer/admin
  before returning; a terminate() failure is best-effort (logged, not fatal) and interrupts
  are preserved.

• No producer leak on failed init. KafkaClientFactory.createProducer closes the producer
  if initTransactions() throws.

Testing

• Unit tests for the retryable-commit classification (including Iceberg vs. Kafka
  CommitFailedException, RebalanceInProgressException, RetriableException) and for the
  consumeAvailable skip-already-consumed-offsets guard.
• Existing coordinator/committer tests cover fenced-vs-fatal termination and the transactional-id
  format.
• Raised the integration-test commit-wait from 30s to 60s to reduce flakiness under CI load.

[kumarpritam863]

@bryanck can you please take a look into this.

[kumarpritam863]

@laskoviymishka can you please check this one.

[emlynazuma]

Alternative to the put() → merge(..., Long::max) line, in case it's useful — handling the rewind
in a ConsumerRebalanceListener so the replay never happens:

@Override public void onPartitionsAssigned(Collection<TopicPartition> partitions) {
  for (TopicPartition partition : partitions) {
    Long inMemoryOffset = controlTopicOffsets.get(partition.partition());
    if (inMemoryOffset != null && inMemoryOffset > consumer.position(partition)) {
      consumer.seek(partition, inMemoryOffset);   // + a warn
    }
  }
}

merge(Long::max) stops the duplicate files, but the replay still happens: the replayed
DataComplete double-count in readyBuffer, so the commit still fires early and validThroughTs()
takes min(timestamp) over only the partitions that reported — the snapshot can claim a
valid-through-ts later than the true watermark. Seeking forward avoids that, and is safe because
the skipped records were consumed by this same Channel and are already in commitBuffer.

Context on #16282. Branch on top of 1.11.0 with regression tests:
apache-iceberg-1.11.0...emlynazuma:iceberg:tongwai/ikc-1.11.0-rebalance-fixes

[kumarpritam863]

@emlynazuma I am adding a proper rebalance consumer listener but I do not think it will be needed if the zombie coordinator scenario is properly handled which after the deterministic changes that I have made will make it almost impossible to have two coordinators running. But to be on the safer side I will add a proper rebalance listener. Also I think the best place to clear thinks are in the revoked partitions. As revoked path is called first if there is any think to revoke so we can just clear the Data Written, Data Complete and control topic offsets stored against those partitions. But I was just analyzing for it to not become complicated. Let me know about your thoughts on these.

[kumarpritam863]

I agree what you have suggested is simpler and more clean. Incorporated that change in the PR. Thanks.

[emlynazuma]

Looks like this landed in onPartitionsAssigned via seekToTrackedOffsets rather than clearing on
revoke — matches what I raised above about revoke-time clearing risking dropping buffered records
that haven't committed yet. And it's still doing work independent of the fencing: open()/close()
starting and stopping the coordinator is itself a -coord membership change, so the rewind can still
happen on a clean single-coordinator handoff, not just during a split-brain window.

Will test this against our repro setup and report back.

[kumarpritam863]

Yes @emlynazuma what you suggested looked clean and sufficient to handle seeking part. Also preventing zombie coordinator in the open and close is much more important than this as that is the reason which is causing this. Also I was thinking that rather than doing all this mumbo-zumbo can't we just do this:

protected void consumeAvailable(Duration pollDuration) {
  ConsumerRecords<String, byte[]> records = consumer.poll(pollDuration);
  while (!records.isEmpty()) {
    for (ConsumerRecord<String, byte[]> record : records) {
      // the consumer stores the offset of the next record to consume,
      // so increment the record offset by one
      if (record.offset() < controlTopicOffsets.getOrDefault(record.partition(), 0L)) {
        LOG.warn("Channel {} processed an already processed offset {} for partition {}", taskId, record.offset(), record.partition());
        continue;
      }
      controlTopicOffsets.put(record.partition(), record.offset() + 1);
      Event event = AvroUtil.decode(record.value());
      if (event.groupId().equals(connectGroupId)) {
        LOG.debug("Received event of type: {}", event.type().name());
        if (receive(new Envelope(event, record.partition(), record.offset()))) {
          LOG.info("Handled event of type: {}", event.type().name());
        }
      }
    }
    records = consumer.poll(pollDuration);
  }
}

This along with the open and close fix and ensuring closed coordinator in the committer should be sufficient I guess. What do you think?

[kumarpritam863]

I will make this change and test if this is suffcient as this will simplyfy and minimize the changes.

[emlynazuma]

A few suggestions now that this has grown quite a bit, meant to help it land faster rather than add
scope:

Split the election rework into its own PR. The fix for the actual reported bug (#16282 —
duplicate files) doesn't depend on how leadership is elected:

• offset-filter dedup in consumeAvailable
• fixed transactional.id fencing
• earliest reset for coordinator recovery
• retryable-commit-error classification

The open()/close() election rewrite (dropping the Admin dependency) is a separate, larger
architectural change with a much bigger surface. Splitting it out would let the duplicate-file fix
land on its own — smaller, more urgent, easier for a committer to review — while the election rework
gets its own PR with more room for scrutiny.

A couple of things worth resolving before requesting review:

• The branch has 39 commits, including some unrelated reverted work and ~24 merge-main commits —
  squashing onto a clean branch would make it much easier to follow what changed and why.

[kumarpritam863]

@emlynazuma I was also thinking to split the PR into two pr's. One the consumeAvailable fix and other the leader election fix. Thanks for the review. Will make that change.

[kumarpritam863]

@laskoviymishka @bryanck @danielcweeks can you please review.

[laskoviymishka]

Nice direction overall — pinning coordinator authority to a stable transactional.id so an epoch bump fences zombies is the right model, and the isRetryable split, the offset-skip filter, and the bounded shutdown are all real improvements that come with tests.

I'd hold this before merging though, because the fencing model it introduces has gaps on both sides of the state machine, and they go straight to the PR's core claim.

The first is the rolling-upgrade window. The new coordinator transactional.id (connectGroupId + "-" + connectorName + "-coord") is a different string from the old prefix + "coordinator" + suffix, and Kafka only fences a producer when the replacement reuses the same id. So during a rolling upgrade the old and new coordinators can't fence each other — both stay live for up to the stop timeout, both broadcast StartCommit, and for a multi-table connector each can win a commit on a different table. SnapshotAncestryValidator won't catch cross-table divergence. I'd like to know whether you intended a migration that fences the old id explicitly, or a hard "full restart required" that's enforced and documented.

The second is re-election after a fence. Once processControlEvents() detects the fence it clears the coordinator and returns, but startCoordinator() only runs from open() on a rebalance. On a stable assignment that leaves the task buffering and writing parquet with no coordinator and no StartCommit, so it silently stops committing while reading as healthy — a data-loss window if retention expires. And the inverse edge worries me too: coordinatorThread.exception() can be null (a requested terminate() never sets it), so a coordinator that stops without a recorded exception hard-fails the task through the else branch.

Things I'd like to settle before merge:

• Fence old-vs-new transactional ids across a rolling upgrade, or enforce+document a full restart
• Give a fenced coordinator a path back to leadership (or at least surface the stall) instead of a silent no-coordinator state
• Guard the null-exception case in processControlEvents, and wire the cause into NotRunningException
• State the intended isRetryable/consecutiveCommitFailures semantics for Kafka-side-only failures
• Bound the shutdown join under Connect's poll/stop deadlines
• Cover the fencing branch with a unit test, and root-cause the integration-timeout bump

A few smaller things are inline. Once the fencing/recovery pieces are settled I'm happy to take another pass and approve.

[laskoviymishka]

The new coordinator transactional.id is a different string from the old transactionalPrefix + "coordinator" + transactionalSuffix, and Kafka fencing only kicks in when a new producer reuses the same id it's replacing.

So during a rolling upgrade an old coordinator (old id) and a new one (new id) don't fence each other — both stay live for up to COORDINATOR_STOP_TIMEOUT_MS, both broadcast StartCommit with different UUIDs, and for a multi-table connector each can win a commit on a different table in the same window. SnapshotAncestryValidator guards a single table against the same base snapshot, but it won't catch two coordinators committing different tables, so cross-table consistency can quietly break.

Which did you intend here — a migration that fences the old id explicitly (init + abort on the old format before switching), or a hard "this upgrade requires a full connector restart, not a rolling one"? Either is defensible, but if it's the latter I'd want it enforced with a startup check and documented, not left implicit. wdyt?

[laskoviymishka]

After the fence is detected we clear the coordinator and return normally — but startCoordinator() is only ever called from open(), which only fires on a Connect rebalance.

On a stable assignment (no rebalance coming) that leaves the task as a pure worker with no coordinator: it keeps writing parquet and buffering, but no StartCommit is ever broadcast, so no Iceberg commit happens and nothing surfaces to Connect's task monitoring. The connector reads as "running" while it has silently stopped committing, and if control-topic or source retention expires while the buffer is held that's a data-loss window.

I get why you moved off throw NotRunningException (Connect won't auto-restart a FAILED task), but the replacement is an unbounded, invisible stall. I'd want either an active re-election trigger (e.g. context.requestCommit() / a health hook) or, at minimum, an ERROR-level "no coordinator active, commits suspended" signal plus an operator-guide note quantifying the stall window. How were you picturing recovery here on a cluster that isn't rebalancing?

[laskoviymishka]

coordinatorThread.exception() can be null — terminate() sets terminated = true but never calls exception.set(...), so a coordinator that stops without recording a throwable lands here with a null cause, isProducerFenced(null) returns false, and we fall into the else and hard-fail the task with NotRunningException.

So this branch treats "terminated but no recorded exception" as fatal, and it can't distinguish a clean/requested stop from a genuine crash. Can you confirm whether a requested terminate can ever reach this check with the field still set? If it can, this crashes the task after a clean stop; if it can't today, it's one refactor away. Either way I'd add an explicit null guard and decide which side null falls on.

While you're here — the new NotRunningException(String, Throwable) ctor is added but this throw site still uses the one-arg form, so the coordinator's actual exception is dropped from the failure. Passing coordinatorThread.exception() as the cause is presumably what the new ctor was for.

[laskoviymishka]

We null coordinatorThread before the join completes, which opens a window. If the join is interrupted we hit the early return with the field already null but the old thread still alive and still holding its producer/consumer — a following open() → startCoordinator() then spins up a second coordinator on the same transactional.id, overlapping the one we never confirmed dead.

The early return also skips the thread.isAlive() warning below, so operators lose the "coordinator still running in the background" signal precisely on the interrupted-shutdown path where it matters most.

I'd only null the field after the join resolves (use the local thread ref plus a boolean to block a second startCoordinator() during the wait), and move the isAlive() check so the interrupt path still logs it.

[laskoviymishka]

The interaction between this allowlist and the single consecutiveCommitFailures counter is under-specified, and it bites from two directions.

Kafka-side retryable errors (RebalanceInProgressException, consumer CommitFailedException, any RetriableException) return true here, so they retry — but note these are thrown from commitConsumerOffsets() after every commitToTable() has already succeeded, and they share one budget with Iceberg OCC conflicts. With max-consecutive-failures = 1, a single RebalanceInProgressException terminates the coordinator even though every table committed fine; conversely a permanent rebalance loop can retry unbounded with no circuit breaker.

What semantics did you intend for a purely Kafka-side failure where the Iceberg commit already landed — reset the counter (it's not an Iceberg-progress failure), or a separate budget? I'd split the two rather than have OCC conflicts and offset-commit hiccups share one threshold.

[laskoviymishka]

UnknownProducerIdException isn't only a fencing signal — it also fires when the broker's transactional state for this id expires (transactional.id.expiration.ms) or on a broker restart, where the right response is re-init, not "a newer coordinator won."

At minimum the log shouldn't assert "fenced by a newer coordinator" for this case — something like "coordinator producer was invalidated (fenced or its transactional state expired)" is honest. The open question is whether you'd rather classify it separately as recoverable and re-init the producer instead of clearing the coordinator — that's more work but avoids the silent-stall path above for the expiry case. wdyt?

[laskoviymishka]

These classification and dedup tests are good, but the behavior this PR is actually about — processControlEvents() clearing a fenced coordinator instead of failing the task — has no test.

I'd add one that starts a CoordinatorThread, injects a ProducerFencedException (or InvalidProducerEpochException) as the thread's exception and marks it terminated, calls processControlEvents(), and asserts coordinatorThread == null with no NotRunningException thrown — and the mirror case (terminated with a non-fencing exception) asserting it does throw. That locks both sides of the branch, including the null-exception edge flagged in processControlEvents.

[laskoviymishka]

Now that CommitFailedException is retryable, the existing testCommitFailedExceptionPropagates passes for a different reason than its name — it only propagates because the mock sets max-consecutive-failures = 1, exhausting the budget on the first try, not because the exception is non-retryable.

I'd rename it to something like testCommitFailedExceptionPropagatesAfterThreshold and assert the retryable path was taken first, so the test validates the new invariant rather than passing by coincidence.

[laskoviymishka]

Worth being precise in the comment about what this filter does, since the PR frames it as dedup. controlTopicOffsets starts empty on every new coordinator, so this only guards against a fetch-position regression within a single coordinator session (e.g. an eager rebalance rewinding the position) — it isn't cross-session idempotency.

The actual cross-session dedup comes from the committedOffsets comparison in commitToTable plus SnapshotAncestryValidator. A one-line comment saying "intra-session guard against position regression, not cross-session dedup" would keep the next reader from over-trusting it.

[laskoviymishka]

Doubling this (and the matching one in TestIntegrationDynamicTable) as "reduce CI flakiness" without a root cause makes future latency regressions invisible. Three changes in this PR add commit-path latency — the bounded join in stopCoordinator(), and especially the coordinator consumer now reading from earliest, which replays control-topic history on Channel.start() and gets worse as the topic grows in production.

If earliest replay is the culprit that's a real production concern, not just a test-timing one. Can you measure which path slowed down and address that (e.g. ensure the -coord group has committed offsets before the wait), then restore or justify the timeout?

[github-actions]

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.