refactor osprey to use confluent-kafka instead of kafka-python#220
refactor osprey to use confluent-kafka instead of kafka-python#220haileyok wants to merge 7 commits into
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
|
||
| self._producer = Producer(config) | ||
| self._threaded_producer = ThreadedKafkaProducer(self._producer) | ||
| self.ensure_topic() |
There was a problem hiding this comment.
It may be worth making this optional, as in some environments you don't want dynamic topic creation, but instead to manage topics with say, terraform (this was a thing we did with IFTAS CCS, such that anything exploiting our code couldn't gain admin access to kafka)
There was a problem hiding this comment.
ya i agree, i might just rip this code out entirely but maybe ill make it some environment variable that you can toggle on...idk yet
| # TODO: num_partitions and replication_factor should not be hard coded... | ||
| topic = NewTopic(self._output_topic, num_partitions=3, replication_factor=3) |
There was a problem hiding this comment.
The only other place num_partitions and replication_factor are mentioned are in test-data producer, they're not set in any environment so this should be okay, except you're thinking of setting it as an environment variable.
There was a problem hiding this comment.
yea this would need to be an environment variable for sure. like emelia's comment though, this is kinda an infra thing that might not belong in here and i might just end up removing it.
| if item is _SENTINEL: | ||
| break |
There was a problem hiding this comment.
This is for my curiosity, why did you do this?
| import threading | ||
| from typing import Any, Optional | ||
|
|
||
| import gevent._threading as _real_threading |
There was a problem hiding this comment.
I'm curious why you did this rather than do what you did with _real_sleep, your commit message didn't say why and it's not obvious.
|
I also think #221 should be included here, would make testing easier and also seeing as its absence didn't break anything on main but testing this without it is impossible. |
|
Tested, and I still notice the same thing reported in #213, I'll test again later. |
3 participants
Description
Will fill out more later, but here I'm removing kafka-python and replacing it with confluent-kafka. Fixes #229
Checklist
uv run ruff check .passes (no unused imports or other lint errors)uv tool run fawltydeps --check-unused --pyenv .venvpasses (no unused dependencies)CHANGELOG.mdwith my changes, if applicable