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

[RSDK-9879] - Make datamanger sync dep implcit #36

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

seanavery
Copy link
Collaborator

@seanavery seanavery commented Jan 31, 2025

Description

When I first wrote video-store, I did not understand implicit dependencies, so just fixing this now. Also, a user recently wanted to use the module without a sync service so wanted to make that possible.

  • Gets rid of hacky explicit datamanager dep check.
  • Supports implicit datamanager dep.
  • Makes sync attribute optional.

Tests

  • Added automated config tests ✅
  • Manual tests:
    • Implicit sync dep succeeds ✅
    • Missing sync attr succeeds ✅
    • Mis-named sync dep fails ✅
    • Sync attr with no service fails ✅

@seanavery seanavery requested review from randhid and hexbabe January 31, 2025 19:05
Copy link
Contributor

@randhid randhid left a comment

Choose a reason for hiding this comment

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

Oh, I too thought data manager could only be added as an explicit dependency.

Heads up - data manager is special - it uses weak dependencies, which are prone to reconfigure loops especially when data manager weakly depends on a component and the component implicitly depends on data manager, creating a cycle, please test with the full suite (re-id, event manage, video store viamrtsp) config for this config change. If it makes it reconfigure in a loop too often.

Because this requires a lot of testing, I'd rather you focus on the rtp passtrough poc. I know this seemed like a quick win, but you're playing with the most finicky part of our dependencies stack here.

Copy link
Collaborator

@hexbabe hexbabe left a comment

Choose a reason for hiding this comment

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

Code LGTM

Also what Rand says makes sense for extra carefulness

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.

3 participants