Skip to content

make kafka topic creation and partition config configurable via env vars - #255

Open
VINODvoid wants to merge 11 commits into
roostorg:mainfrom
VINODvoid:fix/kafka-topic-config
Open

make kafka topic creation and partition config configurable via env vars#255
VINODvoid wants to merge 11 commits into
roostorg:mainfrom
VINODvoid:fix/kafka-topic-config

Conversation

@VINODvoid

Copy link
Copy Markdown
Contributor

Description

Addresses two open review comments on #220.

KafkaOutputSink had hardcoded num_partitions=3 and replication_factor=3 in ensure_topic(), and topic auto-creation always ran unconditionally on startup. This makes both
configurable via environment variables.

OSPREY_KAFKA_AUTO_CREATE_TOPIC (default: true) — set to false for deployments that manage Kafka topics via Terraform or other IaC tooling.

OSPREY_KAFKA_NUM_PARTITIONS (default: 1) and OSPREY_KAFKA_REPLICATION_FACTOR (default: 1) — replaces the hardcoded 3 values. Defaults to 1 so single-broker local
setups (docker-compose) don't fail silently on topic creation. Production deployments can override via env.

Both are read in sink_register.py and passed into KafkaOutputSink.__init__, following the same pattern as all other Kafka config in this codebase.

Branch based on hailey/confluent-kafka.

Checklist

  • Tests pass locally
  • uv run ruff check . passes (no unused imports or other lint errors)
  • uv tool run fawltydeps --check-unused --pyenv .venv passes (no unused dependencies)
  • Updated CHANGELOG.md with my changes, if applicable

@VINODvoid
VINODvoid requested review from a team, EXBreder, ayubun, haileyok and vinaysrao1 as code owners May 14, 2026 09:42
@VINODvoid

Copy link
Copy Markdown
Contributor Author

@haileyok hey, took a stab at addressing the review feedback from @ThisIsMissEm and @chimosky — opened a branch off yours if you want to take a look!

@haileyok

Copy link
Copy Markdown
Member

wow, thank you! I'll take a look at this later today, very much appreciate this!!!

output_topic = config.expect_str('OSPREY_KAFKA_OUTPUT_TOPIC')
bootstrap_servers = config.expect_str_list('OSPREY_KAFKA_BOOTSTRAP_SERVERS')
client_id = config.expect_str('OSPREY_KAFKA_OUTPUT_CLIENT_ID')
auto_create_topic = config.get_bool('OSPREY_KAFKA_AUTO_CREATE_TOPIC', True)

@chimosky chimosky May 25, 2026

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.

In your opening comment, you've said "OSPREY_KAFKA_AUTO_CREATE_TOPIC (default: true) — set to false for deployments that manage Kafka topics via Terraform or other IaC tooling.", but you've set the default here to True and also set auto_create_topic to True.

@cassidyjames

Copy link
Copy Markdown
Member

Hey @VINODvoid, thanks for this contribution! We took another look in our recent working group call, and the consensus is that we do want to see this land. Are you able to resolve conflicts and address the feedback so far?

@reitblatt

Copy link
Copy Markdown
Contributor

@VINODvoid Are you still interested in finishing this PR, or should someone else from the community take it on? Thanks!

@VINODvoid

VINODvoid commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

@reitblatt Yes, still interested; happy to finish it. But I want to check direction first, because I think this is blocked on more than the conflicts.

This is based on hailey/confluent-kafka (#220), which is still a draft and hasn't moved since April. main still uses kafka-python, so the code my change actually touches ; ensure_topic() and the hardcoded num_partitions=3, replication_factor=3 ; doesn't exist on main at all. That means this PR can't land into main without #220 landing first.

@chimosky

Copy link
Copy Markdown
Contributor

I'd made some changes here, you can take a look, tests fail but I just pushed and haven't fixed the tests.

@VINODvoid

VINODvoid commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

@chimosky Can u point to the test , link is not pointed there

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.

5 participants