Skip to content
This repository was archived by the owner on Aug 19, 2025. It is now read-only.

Conversation

@woile
Copy link
Contributor

@woile woile commented Feb 21, 2020

Hey Tom I've been playing around with kafka and I made broadcaster work with it :)
There are some things to polish, but maybe you want to play with it so I'm sharing the code.
For some reason the kafka test gets stuck, I'm not really proficient in async code so I find it hard to debug it, I need some help with it.

I'm providing some extra files that can be removed, but they are there to guide, let me know if you want me to remove them.

There's a kafka with zookeeper implementation in the docker-compose file (made by @marcosschroh), by just typing docker-compose up, you get a full kafka (no ssl) in localhost:9092, this is useful to play with the kafka_example.py

Maybe the test suit could be improved using different docker-compose files, I don't know how do you envision them.

Cheers!

@woile woile force-pushed the master branch 2 times, most recently from 6683a1f to 0fba89a Compare February 21, 2020 17:57
@lovelydinosaur
Copy link
Contributor

🔥 😎 Yas!

Will take a look over this next week and discuss, but just want to give my 👍👍👍 until then.

@woile woile force-pushed the master branch 2 times, most recently from 53aec4f to 1e49670 Compare February 21, 2020 22:32
@lovelydinosaur
Copy link
Contributor

Maybe the test suit could be improved using different docker-compose files, I don't know how do you envision them.

I've no idea. We don't even have CI up and running on this repo yet, and open to suggestions there.

@lovelydinosaur
Copy link
Contributor

I reckon let's not have seperate examples for different backends, and instead just document a few different valid backend URLs in the single example.py case?

@woile
Copy link
Contributor Author

woile commented Feb 24, 2020

Maybe the test suit could be improved using different docker-compose files, I don't know how do you envision them.

I've no idea. We don't even have CI up and running on this repo yet, and open to suggestions there.

Maybe we can use some of this packages:
https://github.com/avast/pytest-docker
https://github.com/pytest-docker-compose/pytest-docker-compose

And docker-compose would be part of the test-requirements.txt

@woile
Copy link
Contributor Author

woile commented Feb 24, 2020

I reckon let's not have seperate examples for different backends, and instead just document a few different valid backend URLs in the single example.py case?

Agreed, but not sure how.

Should we keep examples/ folder and rename kafka_example.py to just example.py? maybe using an env we could choose the backend BACKEND=redis examples/example.py ?

The current ./example.py is harder to get it up and running because it has no doc about how to use it.

@lovelydinosaur
Copy link
Contributor

The current ./example.py is harder to get it up and running because it has no doc about how to use it.

It might make more sense to move it into a /example directory?

Then we can have...

  • /example/templates/...
  • /example/app.py
  • /example/requirements.txt
  • /example/README.md

@lovelydinosaur
Copy link
Contributor

And populate the URL from something like os.environ.get("BROADCAST_URL", "memory://") so that usage is like so...

BROADCAST_URL=...; uvicorn app:app

@lovelydinosaur
Copy link
Contributor

I wouldn't bother with any scripts stuff in the example directory, just a plain README walkthrough.

@woile
Copy link
Contributor Author

woile commented Feb 25, 2020

Perfect, I'll give it a go tomorrow, and then we see how it evolves

@woile woile force-pushed the master branch 2 times, most recently from c9cf447 to 9b41f81 Compare February 25, 2020 11:44
@woile
Copy link
Contributor Author

woile commented Feb 25, 2020

Updated!

Copy link
Contributor

@lovelydinosaur lovelydinosaur left a comment

Choose a reason for hiding this comment

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

Okay, this is looking pretty fantastic actually.

A few very minor suggestions, but probably also happy to merge if any of those are contentious.

@woile
Copy link
Contributor Author

woile commented Feb 26, 2020

Nothing serious, I've updated the PR with the changes

@lovelydinosaur lovelydinosaur merged commit a6a4c7c into encode:master Feb 26, 2020
@lovelydinosaur
Copy link
Contributor

When testing redis (commenting out the other postgres/kafka cases, and using scripts/start redis), the connection establishes to the docker redis-server on the first run, but hangs if I re-run the tests. Are you able to replicate too, and any idea how to resolve?

@woile
Copy link
Contributor Author

woile commented Feb 26, 2020

I'm running pytest -k redis tests/ with no problem several times.

Try removing the volumes from redis when stopping it: docker-compose down -v

@lovelydinosaur
Copy link
Contributor

No dice

@hramezani
Copy link
Contributor

When testing redis (commenting out the other postgres/kafka cases, and using scripts/start redis), the connection establishes to the docker redis-server on the first run, but hangs if I re-run the tests. Are you able to replicate too, and any idea how to resolve?

same for me.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants