nahidupa commented on code in PR #17925:
URL: https://github.com/apache/iceberg/pull/17925#discussion_r3988332927
##########
kafka-connect/kafka-connect/src/main/java/org/apache/iceberg/connect/channel/CommitState.java:
##########
@@ -66,7 +69,16 @@ void addReady(Envelope envelope) {
"Received commit ready when no commit in progress, this can happen
during recovery. Commit ID: {}",
dataComplete.commitId());
} else if (Objects.equals(currentCommitId, dataComplete.commitId())) {
- receivedPartitionCount += dataComplete.assignments().size();
+ // Track which source partitions reported rather than how many responses
arrived. The same
Review Comment:
Agreed. I removed the four-line block locally without adding a replacement;
the current-ID guard and `containsAll(expectedPartitions)` logic are unchanged.
Fresh validation passed all 9 `TestCommitState` tests and the connector's
`spotlessCheck`.
The comment-only cleanup has not been committed or pushed yet, so the
published head is still `65320ff8f`. Leaving this thread open until that diff
is available for review.
##########
kafka-connect/kafka-connect/src/main/java/org/apache/iceberg/connect/channel/Coordinator.java:
##########
@@ -98,8 +100,10 @@ class Coordinator extends Channel {
this.catalog = catalog;
this.config = config;
- this.totalPartitionCount =
- members.stream().mapToInt(desc ->
desc.assignment().topicPartitions().size()).sum();
+ this.expectedPartitions =
Review Comment:
Agreed that the constructor and source of the expected identities need
reconciliation. The chosen order is to wait for #17450 to merge, then adapt
this PR to the actual merged implementation: build the expected
`Set<TopicPartition>` from the same partition-metadata path, capture it
immutably in `Coordinator`, and retain
`reportedPartitions.containsAll(expectedPartitions)`. I will not revert
readiness to a count or restore the removed member-description lookup.
The adaptation will also check that unavailable metadata cannot silently
produce an incomplete expected set; any unresolved startup policy there needs
agreement before integration is considered complete. If the merged dispatch
guard skips same-offset replay, the new-offset duplicate test and direct
readiness tests will remain the discriminating coverage, and the
diagram/mutation evidence will be updated accordingly.
#17450 is still open and unmerged as of 2026-09-11. No speculative
constructor refactor is included now, and this thread remains open pending the
merged-base changes and validation.
--
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]
