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

[PAY-722] Check if audio_tx_hist backfilling complete #4276

Merged
merged 7 commits into from
Nov 4, 2022
Merged

Conversation

dharit-tan
Copy link
Contributor

Description

Adds fields in health check that tell us whether the backfill indexing for audio_tx_history is complete by caching the results of each indexer's check_if_backfilling_complete fns in redis. Also checks the redis cache in the check_if_backfilling_complete functions themselves to return early if we've already determined that indexer is complete.

Tests

Observed behavior in remote-dev that is indexing prod.

Monitoring - How will this change be monitored? Are there sufficient logs / alerts?

Lots of error logs if something goes south.

Copy link
Contributor

@sddioulde sddioulde left a comment

Choose a reason for hiding this comment

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

makes sense to me, but should probably get the eyes of someone who's been closer to this than me too

discovery-provider/src/queries/get_health.py Outdated Show resolved Hide resolved
"index_user_bank_backfill.py | Tried to check if complete, but no stop_sig"
)
return False
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: since the if returns, no need for else

Copy link
Contributor

Choose a reason for hiding this comment

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

i see that you're doing this for the other indexers too... consider updating those too if you want.

also, it seems that the check_if_backfilling_complete all have similar logic. maybe worth considering if can consolidate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they do but gonna leave that for now.

discovery-provider/src/queries/get_health.py Outdated Show resolved Hide resolved
Copy link
Contributor

@piazzatron piazzatron left a comment

Choose a reason for hiding this comment

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

LGTM - only note is that we can simplify some of this if we treat this key as a set instead of a key for a map and just check existence

Comment on lines 632 to 634
redis_inst.delete("index_user_bank_backfill_complete")
redis_inst.delete("index_rewards_manager_backfill_complete")
redis_inst.delete("index_spl_token_backfill_complete")
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think we need to / want to delete these here - don't we want these bools to persist across container restarts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... not sure it matters as they will be persisted in the db anyways.

try:
redis_result = redis.get(index_spl_token_backfill_complete)
if redis_result:
is_spl_token_backfill_complete = str(redis_result.decode())
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe bool?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct me if I'm wrong but I don't think we can store bools in redis, so I'm storing a string that says either 'false' or 'true' and just checking against those values.

Comment on lines 587 to 590
if redis_complete:
redis_complete = str(redis_complete.decode())
if redis_complete == "true":
return True
Copy link
Contributor

@piazzatron piazzatron Nov 4, 2022

Choose a reason for hiding this comment

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

nit: if just checking for the existence of this key in redis, we wouldn't don't need this comparison

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooohhh I see.

Comment on lines 624 to 625
index_rewards_manager_backfill_complete, "true" if complete else "false"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

for simplicity, let's just not set the key at all if the backfill isn't complete. also, 1 or 0 probs a bit nicer convention to represent this kind of thing than stringifying 'true'

@dharit-tan dharit-tan merged commit 4f6eda1 into main Nov 4, 2022
@dharit-tan dharit-tan deleted the rt-PAY-722 branch November 4, 2022 20:04
@AudiusProject AudiusProject deleted a comment from linear bot Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants