feat: add brokers field to support set broker with the same host in kafka-logger plugin#7999
Merged
Merged
Conversation
starsz
force-pushed
the
feat_support_broker_array
branch
from
September 29, 2022 03:58
f3a2f76 to
48d9379
Compare
starsz
changed the title
starsz
marked this pull request as ready for review
September 29, 2022 07:55
tokers
reviewed
Sep 29, 2022
|
|
||
|
|
||
| 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 | |
Contributor
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
| | 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). | |
Member
There was a problem hiding this comment.
It would be better to put the "deprecated" in the Description instead of the Type?
Contributor
Author
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`. | |
Member
There was a problem hiding this comment.
Do we need to update the Chinese version?
CC @SylviaBABY
Member
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
Contributor
Author
There was a problem hiding this comment.
Let me update the Chinese version.
|
|
||
| -- 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 |
Member
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.
Contributor
Author
There was a problem hiding this comment.
Thanks for the remind. Improved.
SylviaBABY
reviewed
Sep 30, 2022
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 <[email protected]>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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