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

Implement resolve_canonical_bootstrap_servers_only #2156

Merged
merged 1 commit into from
Jul 31, 2023
Merged

Implement resolve_canonical_bootstrap_servers_only #2156

merged 1 commit into from
Jul 31, 2023

Conversation

gebn
Copy link
Contributor

@gebn gebn commented Feb 22, 2022

This PR allows specifying the equivalent of client.dns.lookup=resolve_canonical_bootstrap_servers_only, described in KIP-235. The motivation is identical to the one in the proposal: the lack of this feature effectively mandates providing individual canonical broker hostnames when using GSSAPI, rather than a single alias which resolves to the same underlying IPs. This increases the likelihood of errors, and commits application owners to checking for new and removed brokers on an ongoing basis.

The Java client implementation for this can be found here.

The config could be designed as a type DNSLookupStrategy int8 enum, with UseAllDNSIPs and ResolveCanonicalBootstrapServersOnly values, however I went for the bool as there is no precedent for mirroring the Java API, and this seemed much simpler to understand.

I was torn about adding context.Context to resolveCanonicalNames()'s signature. To be consistent with existing code, I've kept this internal. It is not fully supported anyway due to Net.Proxy.Dialer being a proxy.Dialer rather than proxy.ContextDialer.

@ghost ghost added cla-needed and removed cla-needed labels Feb 22, 2022
@gebn gebn changed the title Implement client.dns.lookup=resolve_canonical_bootstrap_servers_only option Implement resolve_canonical_bootstrap_servers_only Feb 22, 2022
client.go Outdated Show resolved Hide resolved
@gebn
Copy link
Contributor Author

gebn commented Feb 27, 2023

@dnwe Please let me know if this is missing anything!

@gebn
Copy link
Contributor Author

gebn commented Jul 12, 2023

@dnwe I've rebased and added Signed-off-by. Still really keen to add this feature :)

@gebn
Copy link
Contributor Author

gebn commented Jul 18, 2023

The Test with Kafka 3.1.2 failure was due to a timeout only 8 tests from the end:

STATUS ELAPSED PACKAGE COVER PASS FAIL SKIP
FAIL 960.07s github.com/Shopify/sarama - 423 8 1
PASS 0.03s github.com/Shopify/sarama/examples/http_server 22.5% 3 0 0
PASS 0.04s github.com/Shopify/sarama/mocks 82.1% 36 0 0

There are no changes here that would affect performance once connected to brokers, so I think that's a false negative. I've rebased, can you re-approve the workflow to give it another go?

@gebn
Copy link
Contributor Author

gebn commented Jul 31, 2023

Thanks! Please let me know if I need to do anything else - I'll hold off rebasing the source branch, as it'll only wait for the checks to be re-approved.

Copy link
Collaborator

@dnwe dnwe left a comment

Choose a reason for hiding this comment

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

LGTM, it would be good if you could also add a unitest around the functionality but I’m happy for you to do that in a follow up PR

@dnwe dnwe merged commit 7d7ac52 into IBM:main Jul 31, 2023
13 checks passed
@dnwe dnwe added the feat label Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants