fix: api_version

[DG-Wangtao]

Hi @doujiang24 , I think I made a mistake in https://github.com/doujiang24/lua-resty-kafka/pull/137 while trying to resolve https://github.com/doujiang24/lua-resty-kafka/issues/135, so I'm creating a new pull request to fix it.

[doujiang24]

could you please describe the bug in detail? I'm not sure I'm catching up with you.

[DG-Wangtao]

if not self.api_version then
        version = self.api_version 
end

First of all, the code means that we use self.api_version as version if we **not** set self.api_version manually. It's obvious an error and it should be we use self.api_version as version only if we set self.api_version.

Secondly,API_VERSION_V0 is useed as default value of self.api_version : https://github.com/doujiang24/lua-resty-kafka/blob/3fbed91d81d4fb32d4dda4316f5f2cba04622633/lib/resty/kafka/producer.lua#L341 The self.api_version will be API_VERSION_V0 if we not set manually.

So I implement it as "use self.api_version as version if it's set manually":

if not (self.api_version == API_VERSION_V0) then
        version = self.api_version 
end

[doujiang24]

okay, I see the problem. but I'm afraid this may not be a proper fix.

actually, we hope to use API_VERSION_V1 as the default api_version here, only for producer send, right? this looks a bit weird, we use the different default values in different places.

I think the proper way is, you specify the api_version in your upper level code, while init the producer.

Or, if you think API_VERSION_V0 is not worthing to support, we could turn the default value to API_VERSION_V1 while init producer. and you need to approve it, i.e. the versions of kafka that support API_VERSION_V0 are too old.

[DG-Wangtao]

• API_VERSION_V1 is used as default before my last pr: https://github.com/doujiang24/lua-resty-kafka/pull/149
• I test the API_VERSION_V0 and got an error that 2023/04/13 04:28:47 [error] 1127#0: *147 [lua] producer.lua:272: buffered messages send to kafka err: closed, retryable: true, topic: audit-log, partition_id: 1, length: 5, context: ngx.timer, maybe it is the same error with the ci test failure in https://github.com/doujiang24/lua-resty-kafka/pull/137.
• API_VERSION_V0 maybe not work in some kafka version, at least not work in your ci test env.
• So I think we should use API_VERSION_V1 as default value , or it's maybe a better way to just use API_VERSION_V1 in https://github.com/doujiang24/lua-resty-kafka/blob/3fbed91d81d4fb32d4dda4316f5f2cba04622633/lib/resty/kafka/producer.lua#L341

[doujiang24]

@DG-Wangtao Thanks for your clearify.

I think the proper way is, you specify the api_version in your upper level code, while init the producer.

does this work for you?

[DG-Wangtao]

yes. it works well if I set api_version while init the producer.

I think use API_VERSION_V1 as default value would be better, thus I create a new branch here:

• https://github.com/DG-Wangtao/lua-resty-kafka/tree/fix/api-version
• https://github.com/DG-Wangtao/lua-resty-kafka/blob/7eebc92d0510625285a82e6c7b2cdd9aad19cbda/lib/resty/kafka/producer.lua#L64
• https://github.com/DG-Wangtao/lua-resty-kafka/blob/7eebc92d0510625285a82e6c7b2cdd9aad19cbda/lib/resty/kafka/producer.lua#L337

I you agree with this idea I will update this pull request.

[DG-Wangtao]

Hi @doujiang24, I think this pull request is ready to merge, do you have any concerns?

[doujiang24]

@DG-Wangtao Sorry for the late. Seems this new branch is better, can it pass the CI? https://github.com/doujiang24/lua-resty-kafka/compare/master...DG-Wangtao:lua-resty-kafka:fix/api-version

[doujiang24]

the code logical in the current patch looks weird, if the new branch passes the CI, I prefer the new branch.

[DG-Wangtao]

Yes, logical in branch https://github.com/DG-Wangtao/lua-resty-kafka/tree/fix/api-version is a better way, which has been running in my env for a long time. I created a new pull request from the branch here #160 an passed the CI.

@doujiang24 Thanks for your work, I'm closing this pull request.