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

feat: export some importent params for kafka-client #7266

Merged
merged 5 commits into from
Jun 20, 2022

Conversation

mikawudi
Copy link
Contributor

export 4 importent params about kafka-producter for kafka-logger plugins
if no problem on this commit, i will edit docs later

Copy link
Member

@spacewander spacewander left a comment

Choose a reason for hiding this comment

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

Let's add some doc for the new fields

@@ -83,6 +83,11 @@ local schema = {
-- in lua-resty-kafka, cluster_name is defined as number
-- see https://github.com/doujiang24/lua-resty-kafka#new-1
cluster_name = {type = "integer", minimum = 1, default = 1},
-- config for lua-resty-kafka, default value is same as lua-resty-kafka
kafka_batch_num = {type = "integer", minimum = 1, default = 200},
Copy link
Member

Choose a reason for hiding this comment

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

Would be better to use producer prefix instead of kafka? Since this plugin is already a kafka logger.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea,i will change it

kafka_batch_num = {type = "integer", minimum = 1, default = 200},
kafka_batch_size = {type = "integer", minimum = 0, default = 1048576},
kafka_max_buffering = {type = "integer", minimum = 1, default = 50000},
kafka_time_linger = {type = "integer", minimum = 1, default = 1}
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we call it flush_time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because this params named “linger.ms” in official kafka client producer and other popular kafka client like sarama,
and i don‘t know why named “flush_time” in resty-kafka......i think call it like “linger” is friendly for user google it....

Copy link
Member

@spacewander spacewander left a comment

Choose a reason for hiding this comment

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

Please make the CI pass, thanks!

@@ -47,6 +47,10 @@ For more info on Batch-Processor in Apache APISIX please refer.
| include_resp_body| boolean | optional | false | [false, true] | Whether to include the response body. The response body is included if and only if it is `true`. |
| include_resp_body_expr | array | optional | | | When `include_resp_body` is true, control the behavior based on the result of the [lua-resty-expr](https://github.com/api7/lua-resty-expr) expression. If present, only log the response body when the result is true. |
| cluster_name | integer | optional | 1 | [0,...] | the name of the cluster. When there are two or more kafka clusters, you can specify different names. And this only works with async producer_type.|
| producer_batch_num | integer | optional | 200 | [1,...] | `batch_num` param in [lua-resty-kafka],merge message and batch send to server, unit is message count |
Copy link
Member

Choose a reason for hiding this comment

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

Should be [lua-resty-kafka](link to lua-resty-kafka)?

@spacewander spacewander merged commit 067b2eb into apache:master Jun 20, 2022
@lijing-21
Copy link
Contributor

Hi @mikawudi , thanks for the contribution!

Here is the Contributor T-shirt form[1], if you're interested, kindly take a look :)

[1] https://github.com/apache/apisix/blob/master/CONTRIBUTING.md#contributor-t-shirt

Liu-Junlin pushed a commit to Liu-Junlin/apisix that referenced this pull request Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants