Use errgroup to write segments to disk in background

[mjh1]

Replace the channel based logic with errgroup, makes it a little simpler and easier to reason about.

[codecov]

Codecov Report

Attention: 30 lines in your changes are missing coverage. Please review.

Comparison is base (1ba6bd7) 52.44519% compared to head (e211fe0) 52.43504%.

Additional details and impacted files

@@                 Coverage Diff                 @@
##                main        #998         +/-   ##
===================================================
- Coverage   52.44519%   52.43504%   -0.01015%     
===================================================
  Files             70          70                 
  Lines           7709        7659         -50     
===================================================
- Hits            4043        4016         -27     
+ Misses          3344        3322         -22     
+ Partials         322         321          -1     

Files	Coverage Δ
video/media.go	100.00000% <100.00000%> (+13.04348%)	⬆️
transcode/transcode.go	35.95506% <23.07692%> (+1.04127%)	⬆️

---

Continue to review full report in Codecov by Sentry.

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

Files	Coverage Δ
video/media.go	100.00000% <100.00000%> (+13.04348%)	⬆️
transcode/transcode.go	35.95506% <23.07692%> (+1.04127%)	⬆️

[emranemran]

Why replace channel logic when it's been working alright? Also not saving that many lines of code IMO

[mjh1]

I just find it much easier to follow and really prefer to avoid channels wherever possible. I don't think the current implementation surfaces errors whereas with errgroup this is just built in.
current:

catalyst-api/transcode/transcode.go

Lines 211 to 214 in 513a513

	err := video.WriteSegmentsToDisk(transmuxTopLevelDir, renditionList, segmentBatch)
	if err != nil {
	return
	}

[emranemran]

I just find it much easier to follow and really prefer to avoid channels wherever possible. I don't think the current implementation surfaces errors whereas with errgroup this is just built in. current:

catalyst-api/transcode/transcode.go

Lines 211 to 214 in 513a513

	err := video.WriteSegmentsToDisk(transmuxTopLevelDir, renditionList, segmentBatch)
	if err != nil {
	return
	}

I'm the opposite - channels just kind of fit nicely into the produce-consumer model and the framework sort of made sense to me and I was able to reason around it. The errors are captured in various stages in the channel implementation and surfaced up (e.g. fail to get segment, or fail to write incorrect number of segments, etc).

What's the test plan if we decide to merge this? The main reason the original change was done was to handle OOM conditions and the refactor meant a lot of the logic around mp4s had to be re-tested i.e. short and very long MP4s will need to be tested.

Some other things that come to mind:

• Resource usage - currently we never exceed 40% per pod during typical usage + mp4 generation.
• Are transcoded segments arriving out of order handled?
• Verifying all renditions are the same length and playable.