-
Notifications
You must be signed in to change notification settings - Fork 336
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[improve] PIP-307: Use assigned broker URL hints during broker reconnection #1208
Merged
Conversation
This file contains 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
Add docker-compose instructions for ELM Add ELM test target Parameterize pulsar version in docker script Update broker health checks in test Allow test method httpDo to return the raw bytes Update test Update test code Update test Cosmetic fixes Add mock counters Rename test Add CI script Minor fix Switch to uber atomic package Test fix Test fix Test fix
heesung-sn
reviewed
Apr 18, 2024
heesung-sn
approved these changes
Apr 18, 2024
BewareMyPower
approved these changes
Apr 23, 2024
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.
Motivation
PIP-307 describes a faster process to reconnect a client to the broker when a topic is unloaded from one broker to another. The server-side implementation is out in 3.2. Client-side support is already checked-in for Java and CPP. This PR adds the respective support to the Golang client. It features improvements to both the producer and consumer, while handling direct and proxied connections alike.
Reference Java and CPP implementations:
Modifications
assignedBrokerUrl
andassignedBrokerUrlTls
to the Protobuf spec ofCommandCloseProducer
andCommandCloseConsumer
connection.handleCloseConsumer
andconnection.handleCloseProducer
all the way through topartitionProducer.reconnectToBroker
andpartitionConsumer.reconnectToBroker
, by adding the URLs to theconnectionClosed
structlookupService.getBrokerAddress
had to be exposed as public methodlookupService.GetBrokerAddress
for this purpose. Furthermore, had to add methodConnection.IsProxied
to allow the reconnection to work properly in the context of proxied connections.Verifying this change
Added integration test
ExtensibleLoadManagerTestSuite.TestTopicUnload
, with subtests for both direct connections and proxied connections. The test requires a different configuration than what we currently have elsewhere, so it runs with its own Docker Compose setup. It launches two brokers having the Extensible Load Manager enabled. The test concurrently:- Produces messages to a topic
- Consumes messages from the topic
- Unloads the topic to the other broker
At the end, it verifies that messages have been successfully delivered both before and after the unloading, while issuing no further topic lookup calls. On that last point, it relies on the topic lookup metrics to perform the verification, as the test framework doesn't have a straightforward way to mock and intercept calls otherwise. This can be a point for further improvement of the test codebase.
Does this pull request potentially affect one of the following parts:
If
yes
was chosen, please highlight the changesDocumentation