Copilot commented on code in PR #13040:
URL: https://github.com/apache/apisix/pull/13040#discussion_r2893027762
##########
apisix/plugins/kafka-logger.lua:
##########
@@ -129,6 +129,15 @@ local schema = {
producer_max_buffering = {type = "integer", minimum = 1, default =
50000},
producer_time_linger = {type = "integer", minimum = 1, default = 1},
meta_refresh_interval = {type = "integer", minimum = 1, default = 30},
+ -- Kafka Produce API version. Default 1 to match lua-resty-kafka.
+ -- Kafka 4.x drops support for magic0 and magic1. Use 2 for Kafka 4.x.
+ api_version = {
+ type = "integer",
+ default = 1,
+ minimum = 0,
+ maximum = 2,
+ description = "Produce API version. Use 2 for Kafka 4.x
compatibility.",
+ },
Review Comment:
PR description says the default `api_version` should be set to `2`, but the
schema default is `1` (and docs also describe `1`). Either update the default
to match the PR intent (Kafka 4.x out-of-the-box) or adjust the PR
description/docs to reflect that users must explicitly set `api_version=2` for
Kafka 4.x.
##########
ci/init-plugin-test-service.sh:
##########
@@ -64,6 +64,20 @@ after() {
# configure clickhouse
echo 'CREATE TABLE default.test (`host` String, `client_ip` String,
`route_id` String, `service_id` String, `@timestamp` String, PRIMARY
KEY(`@timestamp`)) ENGINE = MergeTree()' | curl 'http://localhost:8123/'
--data-binary @-
echo 'CREATE TABLE default.test (`host` String, `client_ip` String,
`route_id` String, `service_id` String, `@timestamp` String, PRIMARY
KEY(`@timestamp`)) ENGINE = MergeTree()' | curl 'http://localhost:8124/'
--data-binary @-
+
+ # Kafka 4.x topic for api_version=2 verification (uses bootstrap-server,
not zookeeper)
+ # Placed at the end so failures don't block other service initialization.
+ # Use a bounded timeout per attempt to avoid long hangs if the broker is
unhealthy.
+ for i in {1..20}; do
+ sleep 5
+ timeout 30s docker exec -i apache-apisix-kafka-server4-kafka4-1
kafka-topics.sh \
+ --create --topic test-kafka4 --bootstrap-server localhost:9092 \
+ --partitions 1 --replication-factor 1 2>/dev/null && break || true
+ done
+
+ if ! docker exec -i apache-apisix-kafka-server4-kafka4-1 kafka-topics.sh
--list 2>/dev/null | grep -q '^test-kafka4$'; then
Review Comment:
The Kafka 4.x topic creation uses `--bootstrap-server localhost:9092` inside
the container, but the broker is configured to advertise
`PLAINTEXT_HOST://localhost:39092`. That advertised address/port won’t be
reachable from inside the container, so `kafka-topics.sh --create` can fail
after the initial metadata fetch. Prefer using the internal listener
(`kafka-server4-kafka4:19092`) as the bootstrap server (and use an explicit
script path, similar to the Bitnami Kafka containers).
```suggestion
timeout 30s docker exec -i apache-apisix-kafka-server4-kafka4-1
/opt/bitnami/kafka/bin/kafka-topics.sh \
--create --topic test-kafka4 --bootstrap-server
kafka-server4-kafka4:19092 \
--partitions 1 --replication-factor 1 2>/dev/null && break ||
true
done
if ! docker exec -i apache-apisix-kafka-server4-kafka4-1
/opt/bitnami/kafka/bin/kafka-topics.sh --list --bootstrap-server
kafka-server4-kafka4:19092 2>/dev/null | grep -q '^test-kafka4$'; then
```
##########
ci/init-plugin-test-service.sh:
##########
@@ -64,6 +64,20 @@ after() {
# configure clickhouse
echo 'CREATE TABLE default.test (`host` String, `client_ip` String,
`route_id` String, `service_id` String, `@timestamp` String, PRIMARY
KEY(`@timestamp`)) ENGINE = MergeTree()' | curl 'http://localhost:8123/'
--data-binary @-
echo 'CREATE TABLE default.test (`host` String, `client_ip` String,
`route_id` String, `service_id` String, `@timestamp` String, PRIMARY
KEY(`@timestamp`)) ENGINE = MergeTree()' | curl 'http://localhost:8124/'
--data-binary @-
+
+ # Kafka 4.x topic for api_version=2 verification (uses bootstrap-server,
not zookeeper)
+ # Placed at the end so failures don't block other service initialization.
+ # Use a bounded timeout per attempt to avoid long hangs if the broker is
unhealthy.
+ for i in {1..20}; do
+ sleep 5
+ timeout 30s docker exec -i apache-apisix-kafka-server4-kafka4-1
kafka-topics.sh \
+ --create --topic test-kafka4 --bootstrap-server localhost:9092 \
+ --partitions 1 --replication-factor 1 2>/dev/null && break || true
+ done
+
+ if ! docker exec -i apache-apisix-kafka-server4-kafka4-1 kafka-topics.sh
--list 2>/dev/null | grep -q '^test-kafka4$'; then
Review Comment:
`kafka-topics.sh --list` is invoked without `--bootstrap-server` (or
`--zookeeper`), which will cause the command to error and makes the topic
existence check always fail. Pass the same bootstrap server used for creation
(e.g., `kafka-server4-kafka4:19092`) so this verification actually works.
```suggestion
if ! docker exec -i apache-apisix-kafka-server4-kafka4-1 kafka-topics.sh
--bootstrap-server localhost:9092 --list 2>/dev/null | grep -q '^test-kafka4$';
then
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
