vbhanuchander-lang commented on PR #17933:
URL: https://github.com/apache/iceberg/pull/17933#issuecomment-5526479540
Thanks @uros-b — and you pointed at the right person: #10351 ("Kafka
Connect: Commit coordination")
is where `consumeAvailable` and `controlTopicOffsets` came from.
@bryanck @danielcweeks a short summary so this is quick to judge, since it
is a one-line change with
a longer argument behind it.
`Channel.consumeAvailable` recorded the consumed position with an
unconditional
`put(partition, offset + 1)`, so re-reading a control topic partition moves
the tracked position
**backwards**. The map is not just local bookkeeping — it feeds
`commitConsumerOffsets()`, which is
the group offset a restarted channel resumes from, and it is merged into
`kafka.connect.offsets` on
the snapshot, which is the watermark the min-offset filter uses on later
commits. Once that watermark
is behind, replayed `DataWritten` envelopes pass the filter, `distinctByKey`
only dedupes within one
commit, and append does no path-level dedup — so the same data files are
committed twice. The
reporter of #17340 saw 149 double-referenced files and ~112k duplicated rows
in production.
**The argument I would most like checked:** `commitToTable` already folds
`controlTopicOffsets()`
with `Long::max` against the last committed offsets, so its only consumers
already treat this value
as "furthest position consumed". Making the map itself monotonic just brings
it in line with that,
and it does not change the offsets recorded on the forward path.
`TestChannel` is new — there was no direct `Channel` test before. It drives
a real `Channel` over a
`MockConsumer`: consume offsets 0-4 to reach a watermark of 5, then seek
back to 1 and deliver a
partial replay ending at 2, which is the shape of the re-read in the report.
One test asserts the
in-memory map, one asserts what the channel actually commits to Kafka. Both
fail on `main` at 3
instead of 5. The partial tail matters — replaying the whole 0-4 range
leaves `put` at 5 and passes
with the bug present.
**One open question for you rather than for the diff:** #17340 also floats a
bounded set of recently
committed file locations in the coordinator, as a content-level backstop for
replays the offset
arithmetic cannot see. I left it out deliberately — it is a design call and
a larger change, and I did
not want it holding up the correctness fix. If you want it, I am happy to
follow up separately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
