[fix][client] Don't pin broker-assigned redirect URL across reconnect retries - #26009
Merged
lhotari merged 1 commit intoJun 12, 2026
Merged
Conversation
… retries Fixes apache#25997 apache#25929 added an explicitHostURI pin to ConnectionHandler so the v5 transaction coordinator's metadata-store discovery re-dials the elected leader on retry. The same change also persisted the broker-assigned redirect URL from CommandCloseProducer/CommandCloseConsumer unload notifications into that pin: a producer/consumer redirected once stayed pinned to that address on every subsequent retry, never falling back to topic lookup. A redirect to a wrong or stale broker (e.g. a stepping-down ExtensibleLoadManagerImpl leader redirecting internal-topic clients to itself) wedged the client permanently with ServiceUnitNotReadyException, causing the testRoleChange flakiness on master. Update the pin in connectionClosed() only for handlers already in explicit-host mode, i.e. the TC metadata-store discovery, which entered it via grabCnx(URI, boolean). The pin update is still needed there: retargetLeader() switches leaders by closing the channel and passing the new leader through connectionClosed(), and a failed first dial to the new leader must not retry against the old one. Lookup-based handlers (producers/consumers) never enter explicit-host mode, so the redirect is honored for the immediate reconnect attempt only, restoring the pre-apache#25929 lookup fallback on failure.
lhotari
approved these changes
Jun 12, 2026
10 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #25997
Motivation
#25929 added an
explicitHostURIpin toConnectionHandlerso the v5 transaction coordinator's metadata-store discovery re-dials the elected leader on retry instead of falling back to the service URL. The same change also persisted the broker-assigned redirect URL fromCommandCloseProducer/CommandCloseConsumerunload notifications (PIP-307) into that pin: a producer/consumer that was redirected once stayed pinned to that address on every subsequent retry, never going through topic lookup again, and nothing ever cleared the field.A redirect to a wrong or stale broker — e.g. a stepping-down
ExtensibleLoadManagerImplleader whosecloseInternalTopics()redirects internal-topic clients to itself via the stale channel-table assignment — now wedged the client permanently withServiceUnitNotReadyExceptioninstead of recovering via lookup on the next retry. This is the cause of theExtensibleLoadManagerImplTest.testRoleChangeflakiness on master. Before #25929 the redirect was honored for the immediate reconnect attempt only, so one failed attempt was followed by a lookup that found the new owner.Modifications
ConnectionHandler.connectionClosed()updates theexplicitHostURIpin only for handlers already in explicit-host mode, i.e. the TC metadata-store discovery, which entered it viagrabCnx(URI, boolean). The pin update is still needed there:TransactionMetaStoreHandler.retargetLeader()switches leaders by closing the channel and passing the new leader URI throughconnectionClosed(), and a failed first dial to the new leader must not retry against the old one.assignedBrokerServiceUrlto a live producer and consumer (exactly asClientCnx#handleCloseProducer/#handleCloseConsumerdo) and asserts both recover via lookup and can exchange a message.Verifying this change
This change added tests and can be verified as follows:
BrokerClientIntegrationTest.testStaleBrokerRedirectFallsBackToLookupfails on master without the fix (the clients stay pinned to the unreachable address) and passes with it.TransactionClientConnectTestpasses, covering the TC client connection path.ExtensibleLoadManagerImplTest.testRoleChangewithinvocationCount = 20passes 40/40 invocations with the fix (the test factory runs bothServiceUnitStateChannelimplementations), vs. 1-2 failures per 20 on master per [Bug][client] Regression in #25929: broker-assigned redirect URL is permanently pinned by ConnectionHandler, breaking lookup fallback on reconnect #25997.