-
Notifications
You must be signed in to change notification settings - Fork 298
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
feat: Support automatic DNS lookup for kafka bootstrap servers #3379
feat: Support automatic DNS lookup for kafka bootstrap servers #3379
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@niebayes I wonder if we should resolve/lookup endpoints on deserialization. It looks more reasonable to resolve/lookup at ClientManager::try_new
, i.e., when we're building the connection.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3379 +/- ##
==========================================
- Coverage 85.58% 84.54% -1.04%
==========================================
Files 892 899 +7
Lines 146543 148952 +2409
==========================================
+ Hits 125413 125926 +513
- Misses 21130 23026 +1896 |
@J0HN50N133 Hi, I've read through your code. It's great. However, I agree with @tisonkun that it's better to perform DNS lookup upon building a Kafka client. Apologize for my previous silly suggestion :). You can browse over the ClientBuilder struct in our codebase and come up with an appropriate solution on DNS lookup. You are supposed to focus on the |
@niebayes I've implemented it without introduce any new crate. |
@J0HN50N133 Please try to avoid delete comments which can increase confusions. You can hide your own comments or edit in place. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good.
@J0HN50N133 @niebayes seems the chaneges are quite duplicate, can we reduce this dup?
I was trying to find a way, but topic_manager.rs and client_manager.rs belong to different crates. I don't know where to put the function to share it and handle the error. |
src/common/meta/src/wal_options_allocator/kafka/topic_manager.rs
Outdated
Show resolved
Hide resolved
I wonder if we can have a gtkafka crate wrap rskafka for our utilities. But I don't know if we share so many code and worth it. Or we just duplicate a small range of code. Wait for the original author @niebayes's suggestion. I'm OK with current duplication level. |
src/common/meta/src/wal_options_allocator/kafka/topic_manager.rs
Outdated
Show resolved
Hide resolved
src/common/meta/src/wal_options_allocator/kafka/topic_manager.rs
Outdated
Show resolved
Hide resolved
@J0HN50N133 Hi, I've post some review comments. Resolve them and we'll merge your PR ASAP. |
@J0HN50N133 Hi, your code look great and work well. However, greptimedb keeps a conservative attitude towards introducing new dependencies, and since the |
src/common/meta/src/wal_options_allocator/kafka/topic_manager.rs
Outdated
Show resolved
Hide resolved
Actually, I didn't introduce mockall. It already exists in the workspace Cargo.toml. |
@J0HN50N133 Sorry, I've searched the codebase and found another usage of the |
@niebayes I've simplified the implementation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: niebayes <niebayes@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@J0HN50N133 Thank you very much for your contribution! Your code has been successfully merged into the greptimedb project. We believe that it will operate reliably across thousands of servers and vehicles. We appreciate your valuable contribution, and please continue to stay engaged with the greptimedb community. |
@J0HN50N133 Take a look at our roadmap 2024. |
FWIW there is an issue for it - Also, @J0HN50N133 you can check other good first issue in https://github.com/GreptimeTeam/greptimedb/contribute. |
@J0HN50N133 If you can share a bit about your background, interests, and goals, I can tag you when I find issues that would be suitable for you. If you don't want to share it publicly, feel free to drop an email to tison@apache.org. |
I hereby agree to the terms of the GreptimeDB CLA
What's changed and what's your intention?
Support automatic DNS lookup for kafka bootstrap servers.
Checklist
Refer to a related PR or issue link
This fixes #3328