search

doujiang24 / lua-resty-kafka

Lua kafka client driver for the Openresty based on the cosocket API
BSD 3-Clause "New" or "Revised" License
801 stars 274 forks source link

Configurable host name resolving #98

Closed alyoshark closed 2 years ago

alyoshark commented 3 years ago

I have a case in my company project that requires resolving hardcoded host names and found quite a few issues with similar need like #5 #27 #28 #52 #57 . So I proceeded to make the following extension to support passing a resolver config table in client.

I run the patch in production, but I find it hard to be tested with automation script, so testing is a bit lacking in this PR. Kindly review to see if it's applicable. Thank you.

doujiang24 commented 3 years ago

@xch91 Thanks, I think the test data can like this way: resolver = { IP = IP }. having runable tests will be better.

alyoshark commented 3 years ago

I wonder how to actually run it though, as setting up a reproducible & disposable Zookeeper + Kafka env is nontrivial at all. For the original test cases do you actually run Kafka locally? Or is it run in a containerized environment?

alyoshark commented 3 years ago

Hi @doujiang24 , kindly suggest how I should proceed if you are free :)

doujiang24 commented 2 years ago

@xch91 Sorry for the delay.

  1. resolver may be a bit confused since it's not a DNS resolver, just a map, from host to ip. maybe, hosts can be better, align to the /etc/hosts?
  2. for the tests, using { ["127.0.0.1"] = "1.1.1.1" } as test config is good enough, we will see the 1.1.1.1 in error log. We just need to make sure the new config works.
alyoshark commented 2 years ago
  1. Now that you mentioned this naming issue, maybe we can keep the name of resolver and take in a function(string): string instead?
  2. That's very reasonable

劳动节快乐！

doujiang24 commented 2 years ago
  1. Now that you mentioned this naming issue, maybe we can keep the name of resolver and take in a function(string): string instead?

@xch91 Sounds reasonable.

alyoshark commented 2 years ago

@doujiang24 please suggest if i should resolve the conflicts or can you rebase and resolve from your side

doujiang24 commented 2 years ago

@xch91 yes, please resolve the conflicts on your side and force push it. otherwise, LGTM. Thanks.

alyoshark commented 2 years ago

@doujiang24 I've rebased and force pushed, but the history got messed up a bit — the handling logic changed in between the 2 commits and I just used the latest version ¯\(ツ)/¯

But I was not able to setup a local Kafka that works with SASL so when I ran the test cases I skipped them, seeking your help to check them again 🤜🤛

alyoshark commented 2 years ago

@doujiang24 got it!

doujiang24 commented 2 years ago

@xch91 Thanks!