You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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
Concentrate all uses of PubSubEndpoint & EventBroadcaster (from fastapi_websocket_pubsub) in opal-server's PubSub (use the shared abstraction rather than the LL objects)
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).
Reuse task management & cleanup functionality
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).
Clean Spaghetti code of OpalServer managing background tasks.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
EventBroadcasterconnection so not to restart process (as a result of recent fix not ignoring connection failure ofEventBroadcaster) - This relies on this PR of fastapi_websocket_pubsubPubSubEndpoint&EventBroadcaster(fromfastapi_websocket_pubsub) in opal-server'sPubSub(use the shared abstraction rather than the LL objects)PubSubresponsible for detecting broadcaster disconnection an triggering the disconnect callbacks (instead of each component taking care of it separately - e.g statistics etc).topics/publisher.py(most of each is just a layer that adds nothing, useful stuff like thePeriodicPublisherwas moved to more appropriate location).OpalServermanaging background tasks.