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: a better fix for the deleted PartInstance problem #1360

Open
wants to merge 2 commits into
base: release51
Choose a base branch
from

Conversation

jstarpl
Copy link
Member

@jstarpl jstarpl commented Jan 14, 2025

About the Contributor

This pull request is posted on behalf of the NRK.

Type of Contribution

This is a:

Bug fix

Current Behavior

#1358 introduced a bugfix for an issue where the current MOS Story for the Next PartInstance is deleted which caused the ingest & playout side to "lock up", unable to create the Playout Model. That fix, while working, is problematic because:

  1. It's not merge-able into release52
  2. It breaks the overall flow of data through the models, by causing a playout-related decision to be made at ingest time.

New Behavior

The fix is reworked into a new algorithm that:

  1. Prevents the deletion of the Segment containing the nextPart instance
  2. Allows the flow to be unchanged up until ensureNextPartIsValid
  3. Allows ensureNextPartIsValid to discover that the next PartInstance is orphaned: deleted and select something else (much like the fix from #1358)
  4. The PartInstance and Segment are deleted later during a clean-up job, because they are no longer linked to the Rundown.

Testing

  • I have added one or more unit tests for this PR
  • I have updated the relevant unit tests
  • No unit test changes are needed for this PR

Affected areas

  • This PR affects the playout and ingest logic in general.

Time Frame

  • We intend to finish the development on this feature in two weeks time.

Other Information

Status

  • PR is ready to be reviewed.
  • The functionality has been tested by the author.
  • Relevant unit tests has been added / updated.
  • Relevant documentation (code comments, system documentation) has been added / updated.

…gment from being removed if it's nexted, it's going to be removed later
@jstarpl jstarpl requested a review from a team as a code owner January 14, 2025 15:32
Copy link

codecov bot commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.98%. Comparing base (baacf0e) to head (b480cc8).

Additional details and impacted files
@@              Coverage Diff              @@
##           release51    #1360      +/-   ##
=============================================
- Coverage      58.00%   57.98%   -0.03%     
=============================================
  Files            525      525              
  Lines          85240    85218      -22     
  Branches        4462     4457       -5     
=============================================
- Hits           49445    49412      -33     
- Misses         35740    35773      +33     
+ Partials          55       33      -22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jstarpl jstarpl requested a review from Julusian January 14, 2025 15:38
Copy link
Contributor

@nytamin nytamin left a comment

Choose a reason for hiding this comment

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

Yes, this better fix looks reasonable to me!

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