refactor(robot-server): Utilize unsubscribe flags for dynamic topics #14620
Conversation
This file contains 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
This is the robot-server groundwork required to refactor MQTT client subscription logic. Instead of relying on the app-shell's ConnectionManager to determine topic subscription status based on component interest, we greatly reduce the notification system's complexity and bug surface by requiring the robort-server to broadcast an unsubscribe flag for dynamic topics (ex, /runs/:runId). The tradeoff here is the app may be subscribed to topics it actually has no interest in, but the overhead for this is insigificant: occasional refetch flags sent by the robot-server that the client doesn't do anything with (in fact, no longer does the client need to send repeated unsubscribe then subsequent resubscribe packets).
DerekMaggio
reviewed
Mar 11, 2024
robot-server/robot_server/service/notifications/publishers/runs_publisher.py
Show resolved
Hide resolved
DerekMaggio
reviewed
Mar 11, 2024
robot-server/robot_server/service/notifications/publishers/runs_publisher.py
Outdated
Show resolved
Hide resolved
DerekMaggio
approved these changes
Mar 11, 2024
sfoster1
requested changes
Mar 11, 2024
robot-server/robot_server/service/notifications/notification_client.py
Outdated
Show resolved
Hide resolved
robot-server/robot_server/service/notifications/publishers/runs_publisher.py
Outdated
Show resolved
Hide resolved
robot-server/robot_server/service/notifications/publishers/runs_publisher.py
Outdated
Show resolved
Hide resolved
robot-server/robot_server/service/notifications/publishers/runs_publisher.py
Outdated
Show resolved
Hide resolved
sfoster1
approved these changes
Mar 12, 2024
mjhuff
added
the
DO NOT MERGE
|
Will merge alongside #14640 |
mjhuff
removed
the
DO NOT MERGE
Carlos-fernandez
pushed a commit
that referenced
this pull request
May 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes EXEC-305
Overview
This PR adds unsubscribe flags for dynamic notification topics, e.g. "/runs/:runId".
Now that the dust has settled around what our push notification can (and can’t) do, it’s clear that the burden of unsubscribing should be on the robot-server, specifically for dynamic topics.
In other words, it’s ok for the app to connect to a broker and remain subscribed to topics that will never change, ex. “/maintenance_runs/current_run” (and be subscribed to those topics as long as the app is connected). Dynamic topics, such as “/runs/:runId”, should tell the app to unsubscribe however, as requiring the broker to stores these topics unnecessarily increases memory load on the robot server during long-lived sessions.
This model is much easier to reason about as opposed to the component interest model that we use now. Bug surface is reduced, too.
Test Plan
unsubscribe: true.Risk assessment
low