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

add Transcoding session ended log message #2685

Merged
merged 4 commits into from
Dec 14, 2022
Merged

add Transcoding session ended log message #2685

merged 4 commits into from
Dec 14, 2022

Conversation

mjh1
Copy link
Contributor

@mjh1 mjh1 commented Dec 9, 2022

What does this pull request do? Explain your changes. (required)

Adding a log line to make it clear on the O end when B ends a session

Specific updates (required)

  • add a log message when transcoding sessions end
  • make transcode loop timeout greater than http push timeout on B so that we're less likely to see the segment loop timeouts for graceful session ends

How did you test each of these updates (required)

I ran B/O/T locally, pushed an rtmp stream, waited 15 seconds or so then stopped the rtmp input, observed the new log message as shown below:

I1209 13:58:03.008992   98749 orchestrator.go:594] manifestID=foo seqNo=5 orchSessionID=2dcd9996 clientIP=127.0.0.1 sender=0x494E245a669C5cb119568E6c59FE82b6fC058db0 Transcoded segment profile=P360p30fps16x9 bytes=300236
I1209 13:58:03.013062   98749 player.go:105] LPMS got HTTP request @ /stream/2dcd9996/P360p30fps16x9/5.ts
I1209 13:58:03.013095   98749 player.go:105] LPMS got HTTP request @ /stream/2dcd9996/P240p30fps16x9/5.ts
I1209 13:58:03.316575   98749 orchestrator.go:995] Transcoding session ended for sessionID=2dcd9996
I1209 13:58:26.796064   98749 roundinitializer.go:124] New round - preparing to initialize round to join active set, current round is 801

Does this pull request close any open issues?

Fixes #2641

Checklist:

@codecov
Copy link

codecov bot commented Dec 9, 2022

Codecov Report

Merging #2685 (d8d57c1) into master (7ea2d42) will increase coverage by 0.00981%.
The diff coverage is 0.00000%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #2685         +/-   ##
===================================================
+ Coverage   56.33537%   56.34518%   +0.00981%     
===================================================
  Files             88          88                 
  Lines          19107       19109          +2     
===================================================
+ Hits           10764       10767          +3     
+ Misses          7758        7757          -1     
  Partials         585         585                 
Impacted Files Coverage Δ
core/orchestrator.go 77.65517% <0.00000%> (-0.21482%) ⬇️
discovery/db_discovery.go 70.75812% <0.00000%> (+1.08303%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ea2d42...d8d57c1. Read the comment docs.

Impacted Files Coverage Δ
core/orchestrator.go 77.65517% <0.00000%> (-0.21482%) ⬇️
discovery/db_discovery.go 70.75812% <0.00000%> (+1.08303%) ⬆️

@@ -36,7 +36,7 @@ import (

const maxSegmentChannels = 4

var transcodeLoopTimeout = 1 * time.Minute
var transcodeLoopTimeout = 70 * time.Second
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the idea here that we were timing out at the same time as the broadcaster does and so timed out before we had a chance for the B to end the session?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's it yep

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a comment that this timeout is set to be greater than B timeout? I'm not sure if we want to have it as tight as possible, +10 sec probably should work fine.

@@ -991,7 +991,9 @@ func (rtm *RemoteTranscoderManager) selectTranscoder(sessionId string, caps *Cap

// ends transcoding session and releases resources
func (node *LivepeerNode) EndTranscodingSession(sessionId string) {
node.endTranscodingSession(sessionId, context.TODO())
logCtx := context.TODO()
clog.V(common.DEBUG).Infof(logCtx, "Transcoding session ended for sessionID=%v", sessionId)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe ended by the Broadcaster to make it explicit?

@mjh1 mjh1 marked this pull request as ready for review December 12, 2022 11:43
Copy link
Contributor

@cyberj0g cyberj0g left a comment

Choose a reason for hiding this comment

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

LGTM after adding to CHANGELOG_PENDING.md.

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.

Add detail to log when a stream ends
3 participants