Use seg duration to time out remote transcoders. #1511

j0sh · 2020-05-21T07:19:26Z

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

As described in #1478 , orchestrator timeouts for a remote T need to be adjusted to handle longer segment durations.

In addition to transmitting the duration from the B to the O in net.SegData , the duration also needs to be made available to the orchestrator's remote transcoder manager to make use of it. The remote transcoder manager follows the Transcoder interface, which offers nowhere to put the duration:

Transcode(job string, fname string, profiles []ffmpeg.VideoProfile) (*TranscodeData, error)

Since both the orchestrator and broadcaster make use of core.SegTranscodingMetadata (CSTMD), which has similar fields as net.SegData, we formalize this relationship [1] and extract mappings ( 1, 2 ) to / from each: the latter for propagation over the wire, and the former for propagation throughout the codebase.

Having the Transcoder interface take a CSTMD effectively subsumes the job and profiles parameters, with CSTMD.ManifestID and CSTMD.Profiles respectively. From there, there is little reason not to also incorporate the fname parameter, resulting in the this interface:

Transcode(md *SegTranscodingMetadata) (*TranscodeData, error)

The O-T protocol is also adjusted to take net.SegData. Although not strictly necessary since the standalone transcoder does not currently require the duration [2], this has the side effect of a small reduction in technical debt for standalone T, due to reuse of the mappings between net.SegData and CSTMD . Any fields that are added to these structs, such as a denominator for the framerate (#1474 (comment)) will be available by default to standalone T. Back-compat logic that is developed within the mappings will also carry over.

[1] We could have opted to carry net.SegData throughout the codebase rather than making use of CSTMD, but the back-compat in net.SegData requires some translation before its fields can be used within the codebase, eg around profiles. This gives us the ability to evolve each structure as needed. We've already done so with the incorporation of the Fname field to CSTMD which isn't necessary over the wire.

[2] The transcoder will need the duration as soon as it's ready to be incorporated into the load balancer.

Specific updates (required)

cmd, core: Typo fixes. 286a520
net: Regenerate proto with segment duration. 5f03718
core, server: Use net.SegData in the O-T protocol. … 4457979
server, net: Send segment duration over the wire. 3358454
server: Refactor test cases to use coreSegMetadata. … fe29742
core: Add input filename to SegTranscodingMetadata. … b9d3907
core, server: Pass SegTranscodingMetadata to transcoder. b1189c8
core: Set remote transcoder timeout based on segment duration. 21568b1

How did you test each of these updates (required)

Unit testing
Manual testing

Does this pull request close any open issues?

Checklist:

README and other documentation updated
Node runs in OSX and devenv
All tests in ./test.sh pass

darkdarkdragon

Looks good, just couple of small suggestions

darkdarkdragon · 2020-05-21T20:18:45Z

server/rpc.go

+		os = segData.Storage[0]
+	}
+
+	dur := time.Duration(int64(segData.Duration) * int64(time.Millisecond))


Suggested change

dur := time.Duration(int64(segData.Duration) * int64(time.Millisecond))

dur := time.Duration(segData.Duration) * time.Millisecond

Fixed in b7a6479

darkdarkdragon · 2020-05-21T20:20:53Z

net/lp_rpc.proto

+    // Deprecated by segData. Job the segment belongs to.
+    string job      = 2;
+
+    // Deprecated by fullProfiles. Set of presets to transcode into.
    bytes profiles = 17;


recommended way of deprecating fields

Suggested change

bytes profiles = 17;

bytes profiles = 17 [deprecated=true];

Very nice - didn't know about this annotation! Actually, kept this line in place since we still "use" it via setting an invalid value to induce failures. Added a comment to the protobuf indicating this in 4d95785

According to the protobuf docs, this is a no-op in all languages except Java, where it emits a warning. My concern is that if golang ever picks this up - golang only has errors, no warnings - this could cause code to stop compiling and the annotation would have to be removed. I'm not entirely sure about the implications of this, so it seems safer to just keep things as-is.

darkdarkdragon · 2020-05-21T20:21:57Z

net/lp_rpc.proto

    bytes profiles = 17;

-    // Transcoding profiles to use. Supersedes `profiles` field 
+    // Deprecated by segData. Transcoding configuration to use.
    repeated VideoProfile fullProfiles = 33;


Suggested change

repeated VideoProfile fullProfiles = 33;

repeated VideoProfile fullProfiles = 33 [deprecated=true];

Made this reserved in 4d95785

darkdarkdragon · 2020-05-21T20:23:04Z

net/lp_rpc.proto

+// Deprecated fields. May still be populated sometimes for back compat
+
+    // Deprecated by segData. Job the segment belongs to.
+    string job      = 2;


recommended way of deprecating field that's not used anymore

Suggested change

string job = 2;

// string job = 2;

reserved 2;

reserved "job";

Good idea, reserved the number in 4d95785 . Didn't reserve the field name since we don't do JSON, so there should not be a conflict. (Can still add that in if you feel it should be there though.)

j0sh · 2020-05-22T19:31:30Z

Addressed comments via fixup, and also made a clean-up pass through the protobuf file to remove trailing whitespace in 98ab212. Cosmetic changes; nothing functional.

darkdarkdragon

LGTM

net/lp_rpc.proto

server/ot_rpc.go

yondonfu

LGTM pending green CI

Enables better reuse. Transcoder now has similar compatibility assurances as B / O without having to re-implement anything.

Makes things simpler overall.

This will simplify the parameterization of the Transcode function.

Cosmetic only; no functional changes.

j0sh · 2020-05-26T22:32:46Z

Rebased, merged

j0sh requested review from jailuthra, darkdarkdragon, yondonfu and kyriediculous May 21, 2020 07:19

darkdarkdragon suggested changes May 21, 2020

View reviewed changes

Base automatically changed from ja/passthruestimate to master May 22, 2020 17:52

darkdarkdragon approved these changes May 23, 2020

View reviewed changes

yondonfu reviewed May 26, 2020

View reviewed changes

net/lp_rpc.proto Outdated Show resolved Hide resolved

server/ot_rpc.go Show resolved Hide resolved

yondonfu mentioned this pull request May 26, 2020

Clean up error handling in O/T protocol #1518

Closed

yondonfu approved these changes May 26, 2020

View reviewed changes

j0sh added 9 commits May 26, 2020 15:30

cmd, core: Typo fixes.

9ce4029

net: Regenerate proto with segment duration.

d8f3cc4

core, server: Use net.SegData in the O-T protocol.

90365cc

Enables better reuse. Transcoder now has similar compatibility assurances as B / O without having to re-implement anything.

server, net: Send segment duration over the wire.

deb3399

server: Refactor test cases to use coreSegMetadata.

98b47e8

Makes things simpler overall.

core: Add input filename to SegTranscodingMetadata.

b1e029a

This will simplify the parameterization of the Transcode function.

core, server: Pass SegTranscodingMetadata to transcoder.

89b721d

core: Set remote transcoder timeout based on segment duration.

b6b25ce

net: Remove trailing whitespace from proto file.

dace5d5

Cosmetic only; no functional changes.

j0sh force-pushed the ja/remotetimeout branch from 89a3acf to dace5d5 Compare May 26, 2020 22:31

j0sh merged commit dace5d5 into master May 26, 2020

j0sh deleted the ja/remotetimeout branch May 26, 2020 22:32

j0sh mentioned this pull request Jun 3, 2020

Add non-integer framerate support #1525

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use seg duration to time out remote transcoders. #1511

Use seg duration to time out remote transcoders. #1511

j0sh commented May 21, 2020

darkdarkdragon left a comment

darkdarkdragon May 21, 2020

j0sh May 22, 2020

darkdarkdragon May 21, 2020

j0sh May 22, 2020

darkdarkdragon May 21, 2020

j0sh May 22, 2020

darkdarkdragon May 21, 2020

j0sh May 22, 2020

j0sh commented May 22, 2020

darkdarkdragon left a comment

yondonfu left a comment

j0sh commented May 26, 2020

	dur := time.Duration(int64(segData.Duration) * int64(time.Millisecond))
	dur := time.Duration(segData.Duration) * time.Millisecond

	repeated VideoProfile fullProfiles = 33;
	repeated VideoProfile fullProfiles = 33 [deprecated=true];

-    string job      = 2;
+    // string job      = 2;
+    reserved 2;
+    reserved "job";

Use seg duration to time out remote transcoders. #1511

Use seg duration to time out remote transcoders. #1511

Conversation

j0sh commented May 21, 2020

darkdarkdragon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j0sh commented May 22, 2020

darkdarkdragon left a comment

Choose a reason for hiding this comment

yondonfu left a comment

Choose a reason for hiding this comment

j0sh commented May 26, 2020