-
Notifications
You must be signed in to change notification settings - Fork 177
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2392 +/- ##
=============================================
Coverage 54.94227% 54.94227%
=============================================
Files 93 93
Lines 19313 19313
=============================================
Hits 10611 10611
Misses 8113 8113
Partials 589 589
Continue to review full report at Codecov.
|
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@leszko Why is longer timeout needed? |
It's only for the stream tester. Here's the context: livepeer/stream-tester#144 |
There was a problem hiding this 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.
common.SegUploadTimeoutMultiplier = 4.0 | ||
common.SegmentUploadTimeout = 8 * time.Second | ||
common.HTTPDialTimeout = 8 * time.Second | ||
common.SegHttpPushTimeoutMultiplier = 4.0 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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:
|
8c632a1
to
3111d01
Compare
Makes a lot of sense! Thanks for explaining |
What does this pull request do? Explain your changes. (required)
Add env variable
LP_EXTEND_TIMEOUTS
which extends all broadcaster's timeouts to8s
.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:
make
runs successfully./test.sh
passREADME and other documentation updated