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

feat: Raise fatal error on linear manifest request update failure #5138

Conversation

dave-nicholas
Copy link
Contributor

Background

We (Sky/Peacock) required the ability to try a different ad stitched manifest upon a manifest request update failure.

Solution

After the initial retry parameters (timeouts and retries) have been exhausted, error immediately and not continue to retry with the same manifest.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2023

Incremental code coverage: 100.00%

@avelad avelad added type: enhancement New feature or request priority: P3 Useful but not urgent labels Apr 18, 2023
avelad
avelad previously approved these changes Apr 18, 2023
@joeyparrish
Copy link
Member

I'm not certain this is necessary. I think you should be able to get the effect you want from the streaming failure callback feature we already have. Are you familiar with that?

@joeyparrish
Copy link
Member

The default behavior is to retry forever for live streams, but you can handle the error and decide whether or not to keep trying or to load something else, based on any criteria you like.

@avelad avelad added the status: waiting on response Waiting on a response from the reporter(s) of the issue label Apr 19, 2023
@dave-nicholas
Copy link
Contributor Author

dave-nicholas commented Apr 20, 2023

Hi @joeyparrish, Thank you for your reply.

I'm not certain this is necessary. I think you should be able to get the effect you want from the streaming failure callback feature we already have. Are you familiar with that?

The streaming.failureCallback is never called on manifest update failures.

@dave-nicholas
Copy link
Contributor Author

To handle manifest updates using a similar callback system to streaming would be to add a callback inside config.manifest.failureCallback and call that on a manifest update failure.

I am happy to do that work and make that change if you wish (or handle it by raising the fatal error in this PR), whatever you want 👍

@joeyparrish
Copy link
Member

Ah, okay. I misunderstood. I will re-read the PR more carefully and get back to you.

@avelad avelad removed the status: waiting on response Waiting on a response from the reporter(s) of the issue label Apr 21, 2023
@joeyparrish joeyparrish merged commit 3ff7ba3 into shaka-project:main Apr 26, 2023
15 checks passed
@vanyaxk
Copy link
Contributor

vanyaxk commented Jul 19, 2023

is there any chance that this could be included in the next 4.3.* iteration, if the 4.4.0 release is not the next one planned?

@joeyparrish
Copy link
Member

"feat" changes (backward-compatible features) go into the next minor release number (4.4.0). Only backward-compatible bug fixes can be cherry-picked to an existing branch and bump the path release number (4.3.x). See https://semver.org/

We have a few issues to resolve before 4.4.0 will be released, but we are close.

@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority: P3 Useful but not urgent status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants