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

Add play check to discprov #918

Merged
merged 2 commits into from Oct 12, 2020
Merged

Add play check to discprov #918

merged 2 commits into from Oct 12, 2020

Conversation

raymondjacobson
Copy link
Member

Trello Card Link

https://trello.com/c/wM570Sl6/1607-add-play-check-to-discprov

Description

Adds a /play_check to discprov that returns the latest created Play in the db. Endpoint takes an optional max_drift query param that is used to check whether to return a 500.

Services

  • Discovery Provider

Does it touch a critical flow like Discovery indexing, Creator Node track upload, Creator Node gateway, or Creator Node file system?

Delete an option.

  • ✅ Nope

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide repro instructions & any configuration.
Include log analysis if applicable.

  1. Local discprov, tested with and without query param

Please list the unit test(s) you added to verify your changes.

  1. get_latest_play_test.py

Copy link
Contributor

@dmanjunath dmanjunath left a comment

Choose a reason for hiding this comment

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

The code looks good overall, had one question. The thing about this is I'm not sure if relying on a play actually happening is the best proxy for this check. Ideally we could just write something into redis like last_indexed_play after the lock has been acquired and we index, and then check that time against current time. We could still reuse drift as the max interval in when a play index should have happened

max_drift: maximum duration in seconds between `now` and the
latest recorded play record to be considered healthy
"""
max_drift = request.args.get("max_drift", type=int)
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a default value? if we just hit play_check this would probably fail cause max_drift is None?

@raymondjacobson
Copy link
Member Author

The code looks good overall, had one question. The thing about this is I'm not sure if relying on a play actually happening is the best proxy for this check. Ideally we could just write something into redis like last_indexed_play after the lock has been acquired and we index, and then check that time against current time. We could still reuse drift as the max interval in when a play index should have happened

I feel like this is the user facing manifestation that "something has gone wrong" and it's how we discovered the last issue we had (the last db play was behind by 4hrs).

I guess I understand your point about wanted to test the indexing vs whether a play actually happened, but I could see indexing happening but being broken (or identity being broken) and then us passing the check "did we index" but still falling behind.

@dmanjunath
Copy link
Contributor

That's a fair point. As long as we have a sane time set for the drift i think this should be okay. Like how do you feel about 5 mins?

@raymondjacobson
Copy link
Member Author

I was gonna say an hour lol :)

@dmanjunath
Copy link
Contributor

oh that's perfect! approving

@raymondjacobson raymondjacobson merged commit 51af2e3 into master Oct 12, 2020
@raymondjacobson raymondjacobson deleted the rj-plays-health-check branch October 12, 2020 07:18
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