Skip to content

chore(error-log-logger): add kafka meta_refresh_interval#8821

Merged
spacewander merged 2 commits into
apache:masterfrom
ronething:fix/kafka_meta_config
Feb 20, 2023
Merged

chore(error-log-logger): add kafka meta_refresh_interval#8821
spacewander merged 2 commits into
apache:masterfrom
ronething:fix/kafka_meta_config

Conversation

@ronething

Copy link
Copy Markdown
Member

Description

Fixes # (issue)

Inspired by PR: #8762

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@ronething
ronething marked this pull request as ready for review February 9, 2023 09:58
| kafka.required_acks | integer | False | 1 | [0, 1, -1] | Number of acknowledgements the leader needs to receive for the producer to consider the request complete. This controls the durability of the sent records. The attribute follows the same configuration as the Kafka `acks` attribute. See [Apache Kafka documentation](https://kafka.apache.org/documentation/#producerconfigs_acks) for more. |
| kafka.key | string | False | | | Key used for allocating partitions for messages. |
| kafka.cluster_name | integer | False | 1 | [0,...] | Name of the cluster. Used when there are two or more Kafka clusters. Only works if the `producer_type` attribute is set to `async`. |
| kafka.meta_refresh_interval | integer | optional | 30 | [1,...] | `refresh_interval` parameter in [lua-resty-kafka](https://github.com/doujiang24/lua-resty-kafka) specifies the time to auto refresh the metadata, in seconds.|

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
| kafka.meta_refresh_interval | integer | optional | 30 | [1,...] | `refresh_interval` parameter in [lua-resty-kafka](https://github.com/doujiang24/lua-resty-kafka) specifies the time to auto refresh the metadata, in seconds.|
| kafka.meta_refresh_interval | integer | False | 30 | [1,...] | `refresh_interval` parameter in [lua-resty-kafka](https://github.com/doujiang24/lua-resty-kafka) specifies the time to auto refresh the metadata, in seconds.|

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

it seems in this disscussion #8762 (comment), the value is optional?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's keep with the same style in this docs.

If we need to change the value from True/False to optional, let's update all of them.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Let's keep with the same style in this docs.

i will change it later.

}],
"kafka_topic": "test2"
"kafka_topic": "test2",
"meta_refresh_interval": 1

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we add a mock test to ensure this config is passed to broker_config?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@spacewander Can we check by the following steps:

  1. log the broker_config info in send_to_kafka function
  2. check if the error log include "refresh_interval":1000 or not?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can verify it without adding any code to the plugin.

--- extra_init_by_lua

Like this, we can inject some code while creating the producer.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ok, i see. But func create_producer is a local func, how can we inject it by extra_init_by_lua?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can inject

return producer:new(broker_list, broker_config, cluster_name)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@spacewander updated, please take a look, thanks.

Signed-off-by: ashing <[email protected]>
@spacewander
spacewander merged commit 0ebc9cb into apache:master Feb 20, 2023
@ronething
ronething deleted the fix/kafka_meta_config branch February 20, 2023 01:36
hongbinhsu added a commit to fitphp/apix that referenced this pull request Feb 23, 2023
* upstream/master:
  fix: limit count plugin conf parameter undefined error (apache#8902)
  chore: remove duplicate kubernetes test case (apache#8882)
  feat: add 'range_id' algorithm for 'request-id' plugin (apache#8790)
  chore(error-log-logger): add kafka meta_refresh_interval (apache#8821)
  chore(deps): bump golang.org/x/net from 0.2.0 to 0.7.0 in /t/grpc_server_example (apache#8881)
  docs: Update getting-started.md (apache#8763)
  fix(admin): fix wrong http code for patch method (apache#8855)
  feat: stream subsystem support tars service discovery (apache#8826)
  feat(ci): implement image caching to reduce ci build time. (apache#8735)
  feat(admin): add head method support to /apisix/admin (apache#8752)
  feat: opentelemetry plugin config collector.address support specify https scheme (apache#8823)
  fix: add admin schema to control_plane config (apache#8809)
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