Skip to content

Fix OpalServer-EvenBroadcaster integration & other OpalServer refactors#632

Closed
roekatz wants to merge 10 commits into
masterfrom
roe/per-10361-fix-opal-server-eventbroadcaster-integration
Closed

Fix OpalServer-EvenBroadcaster integration & other OpalServer refactors#632
roekatz wants to merge 10 commits into
masterfrom
roe/per-10361-fix-opal-server-eventbroadcaster-integration

Conversation

@roekatz

@roekatz roekatz commented Jul 31, 2024

Copy link
Copy Markdown
Contributor
  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.

@netlify

netlify Bot commented Jul 31, 2024

Copy link
Copy Markdown

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
Comment thread packages/requires.txt
idna>=3.3,<4
typer>=0.4.1,<1
fastapi>=0.109.1,<1
fastapi_websocket_pubsub==0.3.7

Copy link
Copy Markdown
Contributor 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

@danyi1212 danyi1212 marked this pull request as draft February 2, 2025 15:22
@zeevmoney

Copy link
Copy Markdown
Contributor

Thanks for this, @roekatz. This draft has been open since July 2024, now conflicts with master, and depends on an unmerged upstream change (permitio/fastapi_websocket_pubsub#82). In the meantime master gained the ignore_broadcaster_disconnected / BROADCAST_CONN_LOSS_BUGFIX_EXPERIMENT_ENABLED path, and the broadcaster reconnect/consistency problem this PR set out to fix is now addressed by the newer, tested PR #915 (PER-15065), which adds pubsub_resilience.py plus reconnect and consistency integration tests. The remaining value here is the OpalServer refactor work (deleting opal_common/topics/publisher.py, removing the redundant PUBLISHER_ENABLED/init_publisher, the server.py task-management cleanup, and the small async_utils/confi helpers) — worth keeping, but a separate, downstream-sensitive change that needs a fresh rebase. We're closing this in favor of #915 for the reconnect fix. Please reopen if you disagree.

@zeevmoney zeevmoney closed this Jun 21, 2026
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.

2 participants