vbhanuchander-lang commented on issue #17637:
URL: https://github.com/apache/iceberg/issues/17637#issuecomment-5611146016
Confirmed this on current `main` — the diagnosis in the report holds exactly
as written.
`close()` gates coordinator shutdown on
`hasLeaderPartition(closedPartitions)`, which opens a fresh
`Admin` and does a live `describeConsumerGroups`. When that runs during the
rebalance that is
revoking the partitions, the described members can carry no assignments,
`findFirstTopicPartition(members)` returns `null`, `containsFirstPartition`
logs "found no partitions
assigned across all members" and returns `false`, and `stopCoordinator()` is
never reached. The
`coordinatorThread == null` guard in `startCoordinator()` is per-instance,
so the next task to
`open()` on a settled group starts a second coordinator without being able
to see the first.
Before writing a patch I would like to agree on the shape, since there is a
design call here.
**The ambiguous case currently fails toward leaving a coordinator running.**
That is the wrong
default: an undetectable leader means we cannot prove we are *not* the
leader, and the cost of
stopping a coordinator we should have kept is a re-election on the next
`open()`, while the cost of
keeping one we should have stopped is two coordinators both broadcasting
`StartCommit`. I would
invert it — if this committer owns a `coordinatorThread` and leadership
cannot be determined, stop it.
**The live `Admin` describe in `close()` looks avoidable entirely.** The
class already captures
`membersWhenWorkerIsCoordinator` at the moment leadership was established,
when the group *was*
stable, and passes it to the `Coordinator` constructor. `close()` could
derive the leader partition
from that snapshot and check whether `closedPartitions` contains it, which
keeps the existing intent —
only stop when actually losing the leader partition, so a partial
cooperative revocation that leaves
the leader partition in place does not churn the coordinator — without
querying a group that is by
definition mid-rebalance at that moment. It also takes an `Admin` round-trip
out of the close path,
where a throwing describe is its own failure mode.
So, concretely:
1. `close()` derives leadership from the stored member snapshot rather than
a fresh describe.
2. If leadership cannot be determined and `coordinatorThread != null`, stop
the coordinator.
Two questions for whoever knows the original intent here:
- Was the live describe in `close()` deliberate — is there a case where the
snapshot from
`open()` is known to be stale in a way that matters?
- Would you rather `close()` stop the coordinator unconditionally whenever
this instance owns one?
That is simpler and strictly safe against duplicates, at the cost of a
coordinator restart on every
partial revocation. I lean toward the snapshot approach to avoid that
churn, but I do not have a
feel for how often partial revocations happen on a real sink.
Happy to put up a PR with a test that drives `CommitterImpl` against an
`Admin` returning members with
empty assignments and asserts the coordinator is stopped — that reproduces
the report at unit level.
Wanted the approach settled first rather than guess.
--
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]
