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

cmd,common,server: Add LP_EXTEND_TIMEOUTS environment variable #2392

Merged
merged 1 commit into from
May 19, 2022

Conversation

leszko
Copy link
Contributor

@leszko leszko commented May 10, 2022

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

Add env variable LP_EXTEND_TIMEOUTS which extends all broadcaster's timeouts to 8s.

This is a hack to fix Test Streams, so the parameter is not exposed as a flag (or in config file). It's planned to be temporary until the poper solution for Test Streams is implemented: livepeer/stream-tester#145

Specific updates (required)

How did you test each of these updates (required)

Added printlns with timeouts and tested on local devnet

Does this pull request close any open issues?

fix livepeer/stream-tester#144

Checklist:

@codecov
Copy link

codecov bot commented May 10, 2022

Codecov Report

Merging #2392 (3111d01) into master (25c2295) will not change coverage.
The diff coverage is 100.00000%.

Impacted file tree graph

@@              Coverage Diff              @@
##              master       #2392   +/-   ##
=============================================
  Coverage   54.94227%   54.94227%           
=============================================
  Files             93          93           
  Lines          19313       19313           
=============================================
  Hits           10611       10611           
  Misses          8113        8113           
  Partials         589         589           
Impacted Files Coverage Δ
common/util.go 55.70470% <ø> (ø)
server/segment_rpc.go 78.75186% <100.00000%> (ø)

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 25c2295...3111d01. Read the comment docs.

Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

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

This looks like a reasonable change to me, but going to tag some network video folks as well for a second look and for awareness.

Copy link
Contributor

@AlexKordic AlexKordic left a comment

Choose a reason for hiding this comment

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

LGTM!

@AlexKordic
Copy link
Contributor

@leszko Why is longer timeout needed?

@leszko
Copy link
Contributor Author

leszko commented May 17, 2022

@leszko Why is longer timeout needed?

It's only for the stream tester. Here's the context: livepeer/stream-tester#144

Copy link
Member

@victorges victorges left a comment

Choose a reason for hiding this comment

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

LGTM, but I'm not sure if I understand the high-level need for this.

Specifically, is there any reason for these test streams to hit the broadcaster timeouts but that not happening for production streams? Does the test streams have anything different than what we see in prod?

The link on livepeer/stream-tester#144 to the discord message seems to be broken (or at least I didn't manage to open it) so I couldn't find the original discussion about this.

cmd/livepeer/livepeer.go Outdated Show resolved Hide resolved
common.SegUploadTimeoutMultiplier = 4.0
common.SegmentUploadTimeout = 8 * time.Second
common.HTTPDialTimeout = 8 * time.Second
common.SegHttpPushTimeoutMultiplier = 4.0
Copy link
Member

Choose a reason for hiding this comment

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

Should this value be any different than the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the same, but anyway I think it's good to set it up here, to make sure that it's still ok when the default changes.

@leszko
Copy link
Contributor Author

leszko commented May 18, 2022

LGTM, but I'm not sure if I understand the high-level need for this.

Specifically, is there any reason for these test streams to hit the broadcaster timeouts but that not happening for production streams? Does the test streams have anything different than what we see in prod?

Test Streams are different than the production. In a sense, that even if you're not "good enough" to transcode the data in production, you still want to see your test stream data.

For example, we have Os which have seg transcoding latency > 2s. Then, all Bs requests times out. And now:

  • For production => this is a good behavior, we don't want to use this O, because it's too slow; then B needs to fail over to another O
  • For test streams => we still do want to see the data for O, just the data will show that the latency is not that good; currently we don't see ANY data for such O, which makes it hard to reason (both for us and for O operator) if there is some problem with test streams or connectivity or it's just that O is slow

@leszko leszko force-pushed the rl/disable-timeouts-for-stream-tester branch from 8c632a1 to 3111d01 Compare May 18, 2022 08:49
@victorges
Copy link
Member

Makes a lot of sense! Thanks for explaining

@leszko leszko merged commit 30b7330 into master May 19, 2022
@leszko leszko deleted the rl/disable-timeouts-for-stream-tester branch May 25, 2022 08:48
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.

Disable timeouts in Broadcaster for test streams
5 participants