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

server,discovery: Allow B to use any O in case none match maxPrice #2999

Merged
merged 5 commits into from
Apr 5, 2024

Conversation

victorges
Copy link
Member

@victorges victorges commented Apr 4, 2024

What does this pull request do? Explain your changes. (required)
This is to allow broadcasters to use any Orchestrator in case their maxPrice configuration
is set to a too low number such that no Orchestrators offer that service.

This is a fallback strategy after implementing the feature that makes the price per pixel
dynamic based on an external currency. It mitigates the risk of the price quote changing
drastically and the maxPrice becoming automatically too low. When that happens, the B
is going to fallback to any O for a while until the Os update their own prices.

This is a re-implementation of #2985, doing it inside the selection algorithm itself so that
we still fallback to other Os in case there are a few Os with price < maxPrice but that still
can't serve the segments (e.g. perf issues).

Specific updates (required)

  • Remove logic on discovery that filtered Os by maxPrice
  • Add logic on selection algorithm to filter by maxPrice
  • At the same time, also include the logic to fallback to ignoring maxPrice, in case none match

How did you test each of these updates (required)

  • ./test.sh
  • Ran a B and 3 Os with different prices locally. Made sure that:
    • B still selects an O matching the maxPrice, if one is available
    • B selects an O higher than maxPrice, if no lower ones are available
    • B switches mid-stream between a lower and higher priced O, in case the lower ones become unavailable

Does this pull request close any open issues?
Implements ENG-1855

Checklist:

Still kept the feature on the db as it had all the tests
already implemented and could still be useful in the future.
I can remove it if preferred though.
While this may not seem useful now since we just convert
them to floats on the probability calculation, it will be
useful later when comparing prices to max price.
@victorges victorges requested a review from leszko April 4, 2024 22:21
Copy link

codecov bot commented Apr 4, 2024

Codecov Report

Attention: Patch coverage is 85.18519% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 57.47485%. Comparing base (119f346) to head (ebb5566).

Additional details and impacted files

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #2999         +/-   ##
===================================================
+ Coverage   57.43773%   57.47485%   +0.03712%     
===================================================
  Files             92          92                 
  Lines          15697       15706          +9     
===================================================
+ Hits            9016        9027         +11     
+ Misses          6082        6079          -3     
- Partials         599         600          +1     
Files Coverage Δ
common/types.go 0.00000% <ø> (ø)
server/selection.go 93.10345% <100.00000%> (+0.05997%) ⬆️
discovery/db_discovery.go 73.51598% <0.00000%> (+0.06465%) ⬆️
server/selection_algorithm.go 89.23077% <90.47619%> (+3.23077%) ⬆️

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 119f346...ebb5566. Read the comment docs.

Files Coverage Δ
common/types.go 0.00000% <ø> (ø)
server/selection.go 93.10345% <100.00000%> (+0.05997%) ⬆️
discovery/db_discovery.go 73.51598% <0.00000%> (+0.06465%) ⬆️
server/selection_algorithm.go 89.23077% <90.47619%> (+3.23077%) ⬆️

Copy link
Contributor

@leszko leszko left a comment

Choose a reason for hiding this comment

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

LGTM. Added one comment, other than that, it looks good.

One thing to keep in mind is that there is one corner case that will work "worse". I think it should not prevent us from merging and deploying it, but something to mention for the sake of awareness.

So, when we send GetOrch requests to all Os, we wait only 500ms for the reply. If no Os replies, then we increase this timeout to 1s. Then, to 2s, and so on.

So, in theory, it's possible that all Os that replies in 500ms will have price higher than max price, but the Os with price lower than max price will reply later. In result, we'll select an O with price higher than max price, while an O with price lower than max price exists.

server/selection_algorithm.go Show resolved Hide resolved
@victorges victorges merged commit eb25467 into master Apr 5, 2024
18 checks passed
@victorges victorges deleted the vg/feat/b-max-price-fallback branch April 5, 2024 18:57
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.

None yet

2 participants