Pankraz76 commented on PR #23947: URL: https://github.com/apache/kafka/pull/23947#issuecomment-6435641584
Thanks @jolshan, and apologies for the overlap with #22680. I only found it after opening this one. The production change is the same in both PRs, so I don't want to compete on the fix itself. I'd like to point out what this PR adds on top, in case any of it is worth keeping: ### 1. End-to-end reproduction of the lost marker `TransactionsTest#testCommitMarkerIsWrittenAfterPartitionComesBackOnline` runs the scenario from KAFKA-20734 on a real 3-broker cluster: - places the partition on a broker other than the transaction coordinator - shuts that broker down and waits until every live broker reports `NO_LEADER` - commits while the partition is offline - restarts the broker and checks that a `read_committed` consumer sees the record On trunk the marker is dropped, so the consumer never sees the record. The unit tests in #22680 cover the cache and channel-manager logic in isolation. This test also covers controller metadata propagation, the `addTxnMarkersToSend` wiring and the resend after the broker comes back. ### 2. A Javadoc contract that names the caller dependency The regression happened because KAFKA-17573 changed what `Optional.empty()` meant while a caller still relied on the old meaning. This PR documents that dependency on `MetadataCache#getPartitionLeaderEndpoint`: > Callers may treat None as a deleted partition, so it must not be returned just because the leader is unavailable. That should make it harder for a future refactor to bring the bug back. ### 3. Test details - The broker registers with a real rack, and the test asserts `new Node(0, "foo0", 9092, "rack0")` instead of using `.setRack(null)`. - Each case in `MetadataCacheTest` is labelled: offline partition, unregistered leader, unknown topic or partition. ### 4. Open questions found while working on this - **Retry loop cost:** while a partition is offline, a parked marker triggers `wakeup()` repeatedly. I measured about 30,000 `getPartitionLeaderEndpoint` calls per second for a single parked marker. Correctness is fine, because the marker goes out as soon as there is a leader, but it may be worth bounding. - **Possible related regression:** `MetadataCache.toCluster` may have a similar KAFKA-17573 regression. - **Backport:** given the Blocker priority, should this go to 4.3 / 4.4? ### Proposal I'm happy to close this PR in favour of #22680. Once that's merged, I'd open a small follow-up with the integration test and the Javadoc wording. I can also file JIRAs for the retry loop and `toCluster` if you think they're worth tracking. -- 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]
