-
Notifications
You must be signed in to change notification settings - Fork 32
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
Use stream metadata request to detect when stream is ready #2306
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement @mjh1
Added one comment, other than that, it looks fine.
maxRedirects: 10, | ||
}); | ||
const body = await res.text(); | ||
if (res.ok && body.includes(`"meta":`)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the stream always include meta:
or can there be a case in which the playback works but the meta:
is not returned?
I'm trying to think if there is any possibility of breaking of what we currently have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That came from Jaron, so hopefully we're good:
If there's a meta member, the stream is active
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jaron confirmed that meta
is only missing in these cases:
Yeah. It's only missing while the stream is offline or in the middle of booting/shutting down
So we just need to double check we don't see a dip in the success rate of the API after rolling out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with some optional nit
@@ -699,6 +699,33 @@ export const triggerCatalystPullStart = | |||
throw new Error(`failed to trigger catalyst pull`); | |||
}; | |||
|
|||
export const getCatalystStreamMetadata = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super-nit: wdyt of:
- Renaming this to
waitCatalystStreamReady
since this function doesn't just make a single request - Moving the
metadataUrl
construction to this function instead, since it is the one that has the logic on how the metadata works. It could receive thestream
object instead of theurl
and simplify usage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yep that sounds good
What does this pull request do? Explain your changes. (required)
Rather than wait for the playback call to finish we can improve the speed of this API by using the metadata call instead. We still make the playback call to ensure ingest ends up in the right region but the metadata endpoint will return a meta object once the stream is ready for playback, which only seems to take a second or two.
Specific updates (required)
How did you test each of these updates (required)
This was tested in staging: Logs from a test in staging
Does this pull request close any open issues?
https://linear.app/livepeer/issue/PS-778
https://linear.app/livepeer/issue/PS-572
Screenshots (optional)
Checklist