feat: Support automatic DNS lookup for kafka bootstrap servers

[J0HN50N133]

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

• I have written the necessary rustdoc comments.
• I have added the necessary unit tests and integration tests.
• This PR does not require documentation updates.

Refer to a related PR or issue link

This fixes #3328

[J0HN50N133]

@niebayes @tisonkun Hey, I've implemented #3328 naively. I'm not sure whether I should handle the deserialization error with snafu. And do we need a way to test toml deserializaton more elegantly?

[tisonkun]

@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]

Codecov Report

Attention: Patch coverage is 95.34884% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 84.54%. Comparing base (1f1d1b4) to head (3b9c26a).
Report is 31 commits behind head on main.

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     

[niebayes]

@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 topic_manager.rs and client_manager.rs files.

[J0HN50N133]

@niebayes I've implemented it without introduce any new crate.

[tisonkun]

@J0HN50N133 Please try to avoid delete comments which can increase confusions.

You can hide your own comments or edit in place.

[tisonkun]

Generally looks good.

@J0HN50N133 @niebayes seems the chaneges are quite duplicate, can we reduce this dup?

[J0HN50N133]

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.

[tisonkun]

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.

[niebayes]

@J0HN50N133 Hi, I've post some review comments. Resolve them and we'll merge your PR ASAP.

[niebayes]

@J0HN50N133 Hi, your code look great and work well. However, greptimedb keeps a conservative attitude towards introducing new dependencies, and since the mockall crate is being introduced for the first time into greptimedb, we will review your code with extra caution.
On the other hand, generic mock crates may potentially generate unnecessary code during compilation, leading to increased build times. Therefore, we opt for simpler testing methods. I have added some suggestions which you can refer to.

[J0HN50N133]

@J0HN50N133 Hi, your code look great and work well. However, greptimedb keeps a conservative attitude towards introducing new dependencies, and since the mockall crate is being introduced for the first time into greptimedb, we will review your code with extra caution. On the other hand, generic mock crates may potentially generate unnecessary code during compilation, leading to increased build times. Therefore, we opt for simpler testing methods. I have added some suggestions which you can refer to.

Actually, I didn't introduce mockall. It already exists in the workspace Cargo.toml.

[niebayes]

@J0HN50N133 Sorry, I've searched the codebase and found another usage of the mockall crate. However, it's still not recommended to use the mockall crate here, since it's too heavy for writing such a simple unit test. Let'
s keep code simpler.

[J0HN50N133]

@niebayes I've simplified the implementation.

[niebayes]

LGTM

[niebayes]

@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]

@niebayes @tisonkun Thanks for your patient and help. I would like to make more contribution, could you give me some advice?

[niebayes]

@J0HN50N133 Take a look at our roadmap 2024.

[tisonkun]

FWIW there is an issue for it -

• GreptimeDB Roadmap 2024 #3412

Also, @J0HN50N133 you can check other good first issue in https://github.com/GreptimeTeam/greptimedb/contribute.

[tisonkun]

@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 [email protected].