feat: add brokers field to support set broker with the same host in kafka-logger plugin #7999
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
starsz
force-pushed
the
feat_support_broker_array
branch
from
f3a2f76 to
48d9379
Compare
starsz
changed the title
tokers
reviewed
Sep 29, 2022
apisix/plugins/kafka-logger.lua
Outdated
| @@ -251,3 +284,4 @@ end | |||
|
|
|||
|
|
|||
| return _M | |||
|
|
|||
| | broker_list | object | True | | | List of Kafka brokers (nodes). | | ||
| | broker_list | deprecated | True | | | Use `brokers` instead. List of Kafka brokers. (nodes). | | ||
| | brokers | array | True | | | List of Kafka brokers (nodes). | | ||
| | brokers.host | string | True | | | The host of Kafka broker | |
There was a problem hiding this comment.
Suggested change
| | brokers.host | string | True | | | The host of Kafka broker | | |
| | brokers.host | string | True | | | The host of Kafka broker, e.g., `192.168.1.1`. | |
tokers
previously approved these changes
Sep 30, 2022
tzssangglass
previously approved these changes
Sep 30, 2022
spacewander
reviewed
Sep 30, 2022
| @@ -37,7 +37,10 @@ It might take some time to receive the log data. It will be automatically sent a | |||
|
|
|||
| | Name | Type | Required | Default | Valid values | Description | | |||
| | ---------------------- | ------- | -------- | -------------- | --------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | | |||
| | broker_list | object | True | | | List of Kafka brokers (nodes). | | |||
| | broker_list | deprecated | True | | | Use `brokers` instead. List of Kafka brokers. (nodes). | | |||
There was a problem hiding this comment.
It would be better to put the "deprecated" in the Description instead of the Type?
There was a problem hiding this comment.
Oh. I refer to the docs of https://github.com/apache/apisix/blob/master/docs/en/latest/plugins/clickhouse-logger.md#attributes.
Let me fix it.
| | broker_list | object | True | | | List of Kafka brokers (nodes). | | ||
| | broker_list | deprecated | True | | | Use `brokers` instead. List of Kafka brokers. (nodes). | | ||
| | brokers | array | True | | | List of Kafka brokers (nodes). | | ||
| | brokers.host | string | True | | | The host of Kafka broker, e.g, `192.168.1.1`. | |
There was a problem hiding this comment.
Do we need to update the Chinese version?
CC @SylviaBABY
There was a problem hiding this comment.
Do we need to update the Chinese version? CC @SylviaBABY
I think we need to update the CN version. But you can merge this first. I'll update the CN version depending on this part later
There was a problem hiding this comment.
Let me update the Chinese version.
apisix/plugins/kafka-logger.lua
Outdated
| @@ -199,15 +225,24 @@ function _M.log(conf, ctx) | |||
| end | |||
|
|
|||
| -- reuse producer via lrucache to avoid unbalanced partitions of messages in kafka | |||
| local broker_list = core.table.new(core.table.nkeys(conf.broker_list), 0) | |||
| local length = conf.broker_list and core.table.nkeys(conf.broker_list) or #conf.brokers | |||
There was a problem hiding this comment.
Just FYI: if we don't need to rewrite the content of table, we can use core.table.clone instead of alloc + insert in loop.
There was a problem hiding this comment.
Thanks for the remind. Improved.
SylviaBABY
reviewed
Sep 30, 2022
Co-authored-by: Sylvia <39793568+SylviaBABY@users.noreply.github.com>
spacewander
approved these changes
Oct 8, 2022
SylviaBABY
approved these changes
Oct 8, 2022
Liu-Junlin
pushed a commit
to Liu-Junlin/apisix
that referenced
this pull request
Nov 4, 2022
…afka-logger plugin (apache#7999) Co-authored-by: Sylvia <39793568+SylviaBABY@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fix #7998
Because the key in the JSON object should be different.
And the original design we use map to store the broker list, so we can't create a broker with the same host.
Now, I add a new filed
brokers(It's an array type), then we can create brokers with the same host.Checklist