Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix OpalServer-EvenBroadcaster integration & other OpalServer refactors #632

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

roekatz
Copy link
Collaborator

@roekatz roekatz commented Jul 31, 2024

  1. Retry initial EventBroadcaster connection so not to restart process (as a result of recent fix not ignoring connection failure of EventBroadcaster) - This relies on this PR of fastapi_websocket_pubsub
  2. Concentrate all uses of PubSubEndpoint & EventBroadcaster (from fastapi_websocket_pubsub) in opal-server's PubSub (use the shared abstraction rather than the LL objects)
  3. Make PubSub responsible for detecting broadcaster disconnection an triggering the disconnect callbacks (instead of each component taking care of it separately - e.g statistics etc).
  4. Reuse task management & cleanup functionality
  5. Remove basically redundant code of topics/publisher.py (most of each is just a layer that adds nothing, useful stuff like the PeriodicPublisher was moved to more appropriate location).
  6. Clean Spaghetti code of OpalServer managing background tasks.

Copy link

netlify bot commented Jul 31, 2024

Deploy Preview for opal-docs canceled.

Name Link
🔨 Latest commit 376acd6
🔍 Latest deploy log https://app.netlify.com/sites/opal-docs/deploys/66e03ee4fee18f0008dbc59f

@roekatz roekatz force-pushed the roe/per-10361-fix-opal-server-eventbroadcaster-integration branch 2 times, most recently from 69854c3 to ac7557e Compare July 31, 2024 15:57
@roekatz roekatz force-pushed the roe/per-10361-fix-opal-server-eventbroadcaster-integration branch from ac7557e to 82f8421 Compare August 15, 2024 16:05
@roekatz roekatz changed the base branch from master to roe/per-10476-write-opal-application-tests August 15, 2024 16:31
@roekatz roekatz force-pushed the roe/per-10476-write-opal-application-tests branch from 089592e to 2dce27b Compare August 29, 2024 11:56
Base automatically changed from roe/per-10476-write-opal-application-tests to master August 29, 2024 12:02
@roekatz roekatz force-pushed the roe/per-10361-fix-opal-server-eventbroadcaster-integration branch from 82f8421 to bd420e0 Compare September 4, 2024 08:49
@roekatz roekatz force-pushed the roe/per-10361-fix-opal-server-eventbroadcaster-integration branch from bd420e0 to fd22880 Compare September 4, 2024 14:15
@roekatz roekatz marked this pull request as ready for review September 4, 2024 15:42
@roekatz roekatz changed the title Fix OpalServer EvenBroadcaster integration Fix OpalServer-EvenBroadcaster integration & other OpalServer refactors Sep 5, 2024
@@ -1,7 +1,6 @@
idna>=3.3,<4
typer>=0.4.1,<1
fastapi>=0.109.1,<1
fastapi_websocket_pubsub==0.3.7
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will be fixed to 0.3.9 once we release fastapi_websocket_pubsub with that fix - https://github.com/permitio/fastapi_websocket_pubsub/pull/82/files

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.

1 participant