Call session end async to avoid unnecessary blocking#2764
Conversation
| if err := EndTranscodingSession(ctx, sess); err != nil { | ||
| clog.Errorf(ctx, "Error completing transcoding session: %q", err) | ||
| } | ||
| go func() { |
There was a problem hiding this comment.
AFAICT you seem to be right that this call doesn't need to block for B.
|
Setting to draft until I fix the unit tests |
Codecov Report
@@ Coverage Diff @@
## master #2764 +/- ##
===================================================
- Coverage 56.28293% 56.26956% -0.01337%
===================================================
Files 88 88
Lines 19171 19172 +1
===================================================
- Hits 10790 10788 -2
- Misses 7789 7792 +3
Partials 592 592
Continue to review full report at Codecov.
|
mjh1
force-pushed
the
mh/async-session-end
branch
from
8c17363 to
f85d5c2
Compare
cyberj0g
left a comment
cyberj0g
left a comment
There was a problem hiding this comment.
LGTM, except the unit test question, if you want my opinion 👍
| goleakOptions := common.IgnoreRoutines() | ||
| // allow enough time for the transcode end goroutines to finish | ||
| goleakOptions = append(goleakOptions, goleak.MaxSleepInterval(5*time.Second), goleak.MaxRetryAttempts(1000)) | ||
| defer goleak.VerifyNone(t, goleakOptions...) |
There was a problem hiding this comment.
Why 1000 retry attempts and 5 seconds delay (I believe it's a delay between attempts, not overall)? I think, the EndTranscodingSession goroutine will finish instantly here. Also maybe pass options to VerifyNone call directly?
There was a problem hiding this comment.
Thanks @cyberj0g. Unfortunately for the tests the communication for the EndTranscodingSession call isn't set up so it simply times out after the 3 second timeout, so that's the reason for waiting. And the other annoying thing is the goleak library starts off with very small microsecond pauses between retries so that's the reason for the high number.
I've looked into a lot of different ways of abstracting this away for the tests without success, I think it'll have to wait for a later PR.
| if err := EndTranscodingSession(ctx, sess); err != nil { | ||
| clog.Errorf(ctx, "Error completing transcoding session: %q", err) | ||
| } | ||
| go func() { |
There was a problem hiding this comment.
AFAICT you seem to be right that this call doesn't need to block for B.
| with: | ||
| path: /github/home/compiled | ||
| key: ${{ runner.os }}-ffmpeg-${{ hashFiles('**/install_ffmpeg.sh') }} | ||
| key: ${{ runner.os }}-ffmpeg-${{ hashFiles('install_ffmpeg.sh') }} |
There was a problem hiding this comment.
What was the reason for this change?
There was a problem hiding this comment.
Builds were failing on master like this: https://github.com/livepeer/go-livepeer/actions/runs/4309780285/jobs/7517555365
It seemed as though the wildcard matching was not working. @hjpotter92 made some changes in this area recently so I'll check with him when he's back but the change I've made has fixed the issue in any case.
3 participants
What does this pull request do? Explain your changes. (required)
Specific updates (required)
How did you test each of these updates (required)
Ran local B/O/T and verified the session end notifications are working asynchronously.
Does this pull request close any open issues?
#2762
Checklist:
makeruns successfully./test.shpass