Skip to content
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

Refresh brokers given list of seed brokers #1781

Merged
merged 1 commit into from Aug 19, 2020

Conversation

justin-chen
Copy link
Contributor

@justin-chen justin-chen commented Aug 18, 2020

RefreshBrokers takes a list of addresses to be used as seed brokers.
Existing broker connections are closed and the updated list of seed brokers will be used for the next metadata fetch.

RefreshBrokers can be used in the case of failovers when the Kafka cluster's DNS would have to be re-resolved

client.go Outdated Show resolved Hide resolved
@justin-chen justin-chen marked this pull request as ready for review August 19, 2020 14:47
@justin-chen justin-chen requested a review from bai as a code owner August 19, 2020 14:47
Copy link
Contributor

@vvuibert vvuibert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@justin-chen justin-chen changed the title Add method to deregister all connected brokers and provide new seedbr… Refresh brokers given list of seed brokers Aug 19, 2020
@justin-chen justin-chen merged commit a1c698e into master Aug 19, 2020
@justin-chen justin-chen deleted the reconnect-with-seed-brokers branch August 19, 2020 15:15
client.go Show resolved Hide resolved
// Existing broker connections are closed and the updated list of seed brokers
// will be used for the next metadata fetch.
RefreshBrokers(addrs []string) error

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how Sarama folks are adding new features, but this looks like it's a breaking change if someone is implementing the interface 🤔

not sure why someone would do that, but the interface is public 🤷

we could add a trick that I saw in the golang code, to add a private method, so no apps could implement the interface

    // A private method to prevent users implementing the
    // interface and so future additions to it will not
    // violate Go 1 compatibility.
    private()

found the blog post: https://blog.golang.org/module-compatibility

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, i'll add a dummy private() method to the interface in another PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😞 this wasn't a good idea at the end :/ the initial intention of private() is if the user needs to use an interface but don't want other people to implement it, and this is not the case for this interface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants