Skip to content

Commit 5f6236a

Browse files
committed
Address review feedback on kafka adapter CI
Restore the mapper re-exports on csp.adapters.kafka that were dropped in favour of __all__. __all__ only governs star imports, so `from csp.adapters.kafka import DateTimeType` had started raising ImportError, breaking examples/03_using_adapters/kafka. Also fix that example's reference to ProtoMessageMapper, which has never existed under that name. Create test topics via the Kafka AdminClient instead of publishing a warm-up record. The old helper wrote a record under key "foo", which test_metadata and test_push_mode both subscribe to. Deterministic creation also lets the broker disable auto-creation, which un-skips test_invalid_topic. Restore delivery coverage in test_basic and test_raw_pubsub. Tail-slicing pub against sub only lines up when the startup gap equals the in-flight lag, and set membership cannot detect loss, duplication or reordering. Both now align on the first record theduplication or reordering. Both now align on the first record theduplication os duplication or reordering. Both now a the broduplication or reordering. Both now align on the first record theduplication or reo_REBALANCE_DELAY_MS=0, bind only 9092 to loopback, and drop the JMX port and the no-op ACL authorizer. Pin the new job's actions to the same SHAs used elsewhere in the workflow, and scope it with read-only permissions and a timeout. Signed-off-by: Tim Paine <3105306+timkpaine@users.noreply.github.com>
1 parent 5cb00c0 commit 5f6236a

13 files changed

Lines changed: 127 additions & 119 deletions

File tree

.github/workflows/build.yml

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -717,6 +717,11 @@ jobs:
717717
- initialize
718718
- build
719719

720+
permissions:
721+
contents: read
722+
723+
timeout-minutes: 30
724+
720725
strategy:
721726
matrix:
722727
os:
@@ -730,25 +735,27 @@ jobs:
730735

731736
steps:
732737
- name: Checkout
733-
uses: actions/checkout@v4
738+
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
734739
with:
735740
submodules: recursive
741+
persist-credentials: false
736742

737743
- name: Set up Python ${{ matrix.python-version }}
738744
uses: ./.github/actions/setup-python
739745
with:
740746
version: '${{ matrix.python-version }}'
747+
cibuildwheel: false
741748

742749
- name: Install python dependencies
743750
run: make requirements
744751

745752
- name: Install test dependencies
746753
shell: bash
747-
run: sudo apt-get install graphviz
754+
run: sudo apt-get update && sudo apt-get install -y graphviz
748755

749756
# Download artifact
750757
- name: Download wheel
751-
uses: actions/download-artifact@v4
758+
uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1
752759
with:
753760
name: csp-dist-${{ runner.os }}-${{ runner.arch }}-${{ matrix.python-version }}
754761

@@ -757,16 +764,16 @@ jobs:
757764
python -m pip install -U *manylinux*.whl
758765
python -m pip install -U --no-deps *manylinux*.whl --target .
759766
767+
# Services declare healthchecks, so --wait blocks until the broker actually accepts connections
760768
- name: Spin up adapter service
761-
run: make dockerup ADAPTER=${{ matrix.adapter }} DOCKERARGS="--wait --wait-timeout 30"
762-
763-
- name: Wait a few seconds after docker images have been spun up
764-
run: sleep 30
769+
run: make dockerup ADAPTER=${{ matrix.adapter }} DOCKERARGS="--wait --wait-timeout 180"
765770

766771
# Run tests
767772
- name: Setup test flags
768773
shell: bash
769-
run: echo "CSP_TEST_$( echo ${{ matrix.adapter }} | awk '{print toupper($0)}' )=1" >> $GITHUB_ENV
774+
env:
775+
ADAPTER: ${{ matrix.adapter }}
776+
run: echo "CSP_TEST_${ADAPTER^^}=1" >> "$GITHUB_ENV"
770777

771778
- name: Python Test Steps
772779
run: make test-py TEST_ARGS="-k ${{ matrix.adapter }}"

Makefile

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,8 @@ tests: test
117117

118118
.PHONY: dockerup dockerps dockerdown initpodmanmac
119119
ADAPTER := kafka
120-
DOCKER_COMPOSE := docker compose # or podman-compose
120+
# Prefer docker, fall back to podman-compose; override with DOCKER_COMPOSE=...
121+
DOCKER_COMPOSE := $(shell command -v docker >/dev/null 2>&1 && echo "docker compose" || echo "podman-compose")
121122
DOCKERARGS :=
122123

123124
initpodmanmac:

ci/kafka/docker-compose.yml

Lines changed: 22 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,40 +1,47 @@
11
# https://github.com/conduktor/kafka-stack-docker-compose
22
---
3-
version: '2.1'
4-
53
services:
64
zoo1:
7-
image: confluentinc/cp-zookeeper:7.3.2
5+
image: confluentinc/cp-zookeeper:7.5.3
86
hostname: zoo1
97
container_name: zoo1
108
ports:
11-
- "2181:2181"
9+
- "127.0.0.1:2181:2181"
1210
environment:
1311
ZOOKEEPER_CLIENT_PORT: 2181
1412
ZOOKEEPER_SERVER_ID: 1
1513
ZOOKEEPER_SERVERS: zoo1:2888:3888
14+
healthcheck:
15+
test: ["CMD-SHELL", "echo ruok | nc -w 2 localhost 2181 | grep imok"]
16+
interval: 5s
17+
timeout: 5s
18+
retries: 24
1619

1720
kafka1:
18-
image: confluentinc/cp-kafka:7.3.2
21+
image: confluentinc/cp-kafka:7.5.3
1922
hostname: kafka1
2023
container_name: kafka1
2124
ports:
22-
- "9092:9092"
23-
- "29092:29092"
24-
- "9999:9999"
25+
- "127.0.0.1:9092:9092"
2526
environment:
26-
KAFKA_ADVERTISED_LISTENERS: INTERNAL://kafka1:19092,EXTERNAL://${DOCKER_HOST_IP:-127.0.0.1}:9092,DOCKER://host.docker.internal:29092
27-
KAFKA_LISTENER_SECURITY_PROTOCOL_MAP: INTERNAL:PLAINTEXT,EXTERNAL:PLAINTEXT,DOCKER:PLAINTEXT
27+
KAFKA_ADVERTISED_LISTENERS: INTERNAL://kafka1:19092,EXTERNAL://${DOCKER_HOST_IP:-127.0.0.1}:9092
28+
KAFKA_LISTENER_SECURITY_PROTOCOL_MAP: INTERNAL:PLAINTEXT,EXTERNAL:PLAINTEXT
2829
KAFKA_INTER_BROKER_LISTENER_NAME: INTERNAL
2930
KAFKA_ZOOKEEPER_CONNECT: "zoo1:2181"
3031
KAFKA_BROKER_ID: 1
3132
KAFKA_LOG4J_LOGGERS: "kafka.controller=INFO,kafka.producer.async.DefaultEventHandler=INFO,state.change.logger=INFO"
3233
KAFKA_OFFSETS_TOPIC_REPLICATION_FACTOR: 1
3334
KAFKA_TRANSACTION_STATE_LOG_REPLICATION_FACTOR: 1
3435
KAFKA_TRANSACTION_STATE_LOG_MIN_ISR: 1
35-
KAFKA_JMX_PORT: 9999
36-
KAFKA_JMX_HOSTNAME: ${DOCKER_HOST_IP:-127.0.0.1}
37-
KAFKA_AUTHORIZER_CLASS_NAME: kafka.security.authorizer.AclAuthorizer
38-
KAFKA_ALLOW_EVERYONE_IF_NO_ACL_FOUND: "true"
36+
# Consumer groups are created per-run, so skip the 3s default rebalance debounce
37+
KAFKA_GROUP_INITIAL_REBALANCE_DELAY_MS: 0
38+
# Tests create their topics explicitly; test_invalid_topic depends on this being off
39+
KAFKA_AUTO_CREATE_TOPICS_ENABLE: "false"
40+
healthcheck:
41+
test: ["CMD-SHELL", "kafka-broker-api-versions --bootstrap-server localhost:9092"]
42+
interval: 5s
43+
timeout: 10s
44+
retries: 24
3945
depends_on:
40-
- zoo1
46+
zoo1:
47+
condition: service_healthy

conda/dev-environment-unix.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ dependencies:
1717
- flex
1818
- graphviz
1919
- gtest
20-
- httpx>=0.20,<1
2120
- libarrow<24
2221
- libboost>=1.80.0
2322
- libboost-headers>=1.80.0
@@ -41,6 +40,7 @@ dependencies:
4140
- pytest-sugar
4241
- python>=3.10,<3.15
4342
- python-build
43+
- python-confluent-kafka
4444
- python-graphviz
4545
- python-rapidjson
4646
- pytz

conda/dev-environment-win.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ dependencies:
1717
# - flex # not available on windows
1818
- graphviz
1919
- gtest
20-
- httpx>=0.20,<1
2120
- libarrow<24
2221
- libboost>=1.80.0
2322
- libboost-headers>=1.80.0
@@ -41,6 +40,7 @@ dependencies:
4140
- pytest-sugar
4241
- python>=3.10,<3.14
4342
- python-build
43+
- python-confluent-kafka
4444
- python-graphviz
4545
- python-rapidjson
4646
- pytz

csp/adapters/kafka.py

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,29 @@
88
import csp
99
from csp import ts
1010
from csp.adapters.status import Status
11-
from csp.adapters.utils import MsgMapper, hash_mutable
11+
from csp.adapters.utils import (
12+
BytesMessageProtoMapper,
13+
DateTimeType,
14+
JSONTextMessageMapper,
15+
MsgMapper,
16+
RawBytesMessageMapper,
17+
RawTextMessageMapper,
18+
hash_mutable,
19+
)
1220
from csp.impl.wiring import ReplayMode, input_adapter_def, output_adapter_def, status_adapter_def
1321
from csp.lib import _kafkaadapterimpl
1422

15-
__all__ = ("KafkaStatusMessageType", "KafkaStartOffset", "KafkaAdapterManager")
23+
__all__ = (
24+
"KafkaAdapterManager",
25+
"KafkaStartOffset",
26+
"KafkaStatusMessageType",
27+
# re-exported from csp.adapters.utils for backwards compatibility
28+
"BytesMessageProtoMapper",
29+
"DateTimeType",
30+
"JSONTextMessageMapper",
31+
"RawBytesMessageMapper",
32+
"RawTextMessageMapper",
33+
)
1634

1735
T = TypeVar("T")
1836

csp/impl/wiring/adapters.py

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import inspect
22
from datetime import timedelta
3-
from typing import List, TypeVar
3+
from typing import List
44

55
from typing_extensions import override
66

@@ -14,7 +14,6 @@
1414
from csp.impl.wiring.signature import Signature
1515

1616
_ = ReplayMode
17-
T = TypeVar("T")
1817

1918

2019
# Every AdapterDefMeta instance represents an input or output adapter *definition* type

csp/tests/adapters/conftest.py

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
from uuid import uuid4
2+
13
import pytest
24

35
from csp.adapters.kafka import KafkaAdapterManager
@@ -11,13 +13,10 @@ def kafkabroker():
1113

1214
@pytest.fixture(scope="module", autouse=True)
1315
def kafkaadapterkwargs(kafkabroker):
14-
return dict(broker=kafkabroker, group_id="group.id123", rd_kafka_conf_options={"allow.auto.create.topics": "true"})
16+
# Unique group id so a rerun never inherits committed offsets from a previous run
17+
return dict(broker=kafkabroker, group_id=f"csp.test.{uuid4()}")
1518

1619

1720
@pytest.fixture(scope="module", autouse=True)
18-
def kafkaadapter(kafkabroker):
19-
group_id = "group.id123"
20-
_kafkaadapter = KafkaAdapterManager(
21-
broker=kafkabroker, group_id=group_id, rd_kafka_conf_options={"allow.auto.create.topics": "true"}
22-
)
23-
return _kafkaadapter
21+
def kafkaadapter(kafkaadapterkwargs):
22+
return KafkaAdapterManager(**kafkaadapterkwargs)

csp/tests/adapters/kafka_utils.py

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,16 @@
1-
from datetime import datetime, timedelta
1+
__all__ = ("create_topic",)
22

3-
import csp
4-
from csp.adapters.utils import DateTimeType, JSONTextMessageMapper
53

6-
__all__ = ("_precreate_topic",)
4+
def create_topic(broker, topic):
5+
"""Create `topic` and block until the broker acknowledges it.
76
7+
Creation is done out of band rather than by publishing a warm-up message, so that no test data
8+
lands on the topic and so tests do not depend on broker-side auto-creation (disabled in
9+
ci/kafka/docker-compose.yml so test_invalid_topic can exercise the failure path).
10+
"""
11+
# Imported lazily so collection does not require confluent-kafka when the kafka tests are skipped
12+
from confluent_kafka.admin import AdminClient, NewTopic
813

9-
def _precreate_topic(adapter, topic):
10-
"""Since we test against confluent kafka, just use the kafka rest addon"""
11-
12-
def g():
13-
msg_mapper = JSONTextMessageMapper(datetime_type=DateTimeType.UINT64_MICROS)
14-
adapter.publish(msg_mapper, topic, "foo", csp.const("test"), field_map="a")
15-
16-
csp.run(g, starttime=datetime.utcnow(), endtime=timedelta(), realtime=True)
14+
admin = AdminClient({"bootstrap.servers": broker})
15+
for _, future in admin.create_topics([NewTopic(topic, num_partitions=1, replication_factor=1)]).items():
16+
future.result()

0 commit comments

Comments
 (0)