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

[CON-457] add retry and exponential backoff to ensurePrimaryMiddlware #4171

Merged
merged 6 commits into from
Oct 26, 2022

Conversation

jonaylor89
Copy link
Contributor

@jonaylor89 jonaylor89 commented Oct 25, 2022

Description

This PR adds retry logic to ensurePrimaryMiddleware with an exponential backoff to reduce strain on discovery nodes

Tests

No extra tests needed since each individual piece seems to be tested well enough

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

This change can be monitored with content node logs to make sure it actually retries as well as discovery provider grafana metrics more generally to see if there is in fact less strain.

@jonaylor89 jonaylor89 changed the title [CON-471] add retry and exponential backoff to ensurePrimaryMiddlware [CON-457] add retry and exponential backoff to ensurePrimaryMiddlware Oct 25, 2022
@jonaylor89 jonaylor89 marked this pull request as ready for review October 25, 2022 18:44
Copy link
Contributor

@SidSethi SidSethi left a comment

Choose a reason for hiding this comment

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

hahah this will make things much worse - sorry the card was not very clear, but getReplicaSetSpIdsByUserId internally makes retries with a fixed delay (check out that code)
the scope of this is to actually replace that retry logic with asyncRetry

@jonaylor89
Copy link
Contributor Author

hahah this will make things much worse - sorry the card was not very clear, but getReplicaSetSpIdsByUserId internally makes retries with a fixed delay (check out that code) the scope of this is to actually replace that retry logic with asyncRetry

Oh god it does have its own retry log RIP - reverting and taking another crack at it

@pull-request-size pull-request-size bot added size/L and removed size/S labels Oct 25, 2022
Copy link
Contributor

@SidSethi SidSethi left a comment

Choose a reason for hiding this comment

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

need to change values, otherwise lgtm
this is critical path change (every user write) - good to ensure things seem ok locally and on staging

creator-node/src/services/ContentNodeInfoManager.ts Outdated Show resolved Hide resolved
creator-node/src/services/ContentNodeInfoManager.ts Outdated Show resolved Hide resolved
@jonaylor89 jonaylor89 merged commit 0b3c754 into main Oct 26, 2022
@jonaylor89 jonaylor89 deleted the jn-con-471 branch October 26, 2022 13:48
@AudiusProject AudiusProject deleted a comment from linear bot Sep 11, 2023
@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

2 participants