B re-use last session if it passes latency score threshold check #2666
Conversation
yondonfu
changed the title
Codecov Report
@@ Coverage Diff @@
## master #2666 +/- ##
===================================================
+ Coverage 56.30905% 56.36440% +0.05535%
===================================================
Files 88 88
Lines 19052 19067 +15
===================================================
+ Hits 10728 10747 +19
+ Misses 7741 7737 -4
Partials 583 583
Continue to review full report at Codecov.
|
| @@ -704,7 +733,9 @@ func TestTranscodeSegment_CompleteSession(t *testing.T) { | |||
| // Create stub server | |||
| ts, mux := stubTLSServer() | |||
| defer ts.Close() | |||
| transcodeDelay := 100 * time.Millisecond | |||
There was a problem hiding this comment.
I'd try to make unit tests time-dependent, because the test is inherently flaky. Especially that 100ms is very low time, so this test may fail in many environments.
There was a problem hiding this comment.
Agreed that throwing in these types of sleeps should be avoided, but in this case...
- I needed to manipulate the value of the
LatencyScorefield calculated here. I can directly control the value ofseg.Duration, but iftookAllDuris determined by the amount of time that elapses during the request so iftookAllDuris 0 or a value really close to 0 regardless of what I set asseg.DurationI could end up with a 0 value forLatencyScore. So, AFAIK adding a sleep in the/segmenthandler that is triggered during the request was the easiest way to also manipulate the value oftookAllDur. - I actually don't think the test will be flaky with the updates (see the snippet below) to the tests in 8cbd6a2 (I rebased + force-pushed) because the values of
tookAllDurandseg.Durationare set so that we can always guarantee that the calculatedLatencyScoreis what we want for the assertions we are making (i.e. a score <SELECTOR_LATENCY_SCORE_THRESHOLDand a score >SELECTOR_LATENCY_SCORE_THRESHOLD).
segDurMultiplier := 100.0
// The seg duration that meets the latency score threshold
// A larger seg duration gives a smaller/better latency score
// A smaller seg duration gives a larger/worse latency score
segDurLatencyScoreThreshold := transcodeDelay.Seconds() / SELECTOR_LATENCY_SCORE_THRESHOLD
// The seg duration that is better than the threshold (i.e. "fast")
segDurFastLatencyScore := segDurLatencyScoreThreshold * segDurMultiplier
// The seg duration that is worse than the threshold (i.e "slow")
segDurSlowLatencyScore := segDurLatencyScoreThreshold / segDurMultiplier
| // Re-use last session if oldest segment is in-flight for < durMult * segDur | ||
| return session | ||
| // A session in the exclusion list is not selectable | ||
| if includesSession(exclude, session) { |
There was a problem hiding this comment.
Even though it's a side effect of your change, I really like the readability of having these broken out into distinct checks
There was a problem hiding this comment.
Glad you agree that this is more readable! TBH I was having trouble parsing the one-liner with multiple logical operators so felt compelled to do the small refactor.
Increase readability of the func with var names + in-line comments.
yondonfu
force-pushed
the
yf/eager-selection-fix
branch
from
a36a819
to
10afef8
Compare
Tracking in #2669 |
What does this pull request do? Explain your changes. (required)
This PR implements the solution described in #2660.
Outside of this PR I think it would be a good idea to do a refactor to move as much selection logic (i.e. last session tracking, segment in flight tracking, etc.) into MinLSSelector in selection.go to keep that logic encapsulated in a single place. This felt like a hefty change though so I opted not to do that refactor in this PR.
We may also want to separately consider tweaking the value of the constant
SELECTOR_LATENCY_SCORE_THRESHOLDto be slightly higher i.e. 1.1, 1.2 to be more lenient with the latency score threshold check, at least until we get to implementing #1232, if we still end up seeing a lot of swapping between Os during selection - I think the probability of this happening will be higher in scenarios with short segments (i.e. 1s) and lots of rendition profiles which would increase transcode + download time.Specific updates (required)
See commit history.
8dcfaf4 and 5be5fe8 contain small code refactors.
3fc43f3 contains the main update.
How did you test each of these updates (required)
Updated unit tests.
Does this pull request close any open issues?
Fixes #2660
Checklist:
makeruns successfully./test.shpass