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

Fog postgres retries ii (#1352) (backport to nascent release-1.1.3 branch) #148

Merged

Conversation

cbeck88
Copy link
Contributor

@cbeck88 cbeck88 commented Feb 1, 2022

  • This adds configurable retries to postgres operations

Retries are added at the level of the SqlRecoveryDb object.

We also adjust some retry logic for fog ingest controller

  • Redevelop previous per Eran's comment, to more systematically retry

This adds the retries at the level of the SqlRecoveryDb object
to systematically apply retries to all postgres operations.

  • fixup missing "should_retry()" calls

  • fix clippy

This repository is no longer used, please rebase your PR against https://github.com/mobilecoinfoundation/mobilecoin.git.

Thanks!

* This adds configurable retries to postgres operations

Retries are added at the level of the SqlRecoveryDb object.

We also adjust some retry logic for fog ingest controller

* Redevelop previous per Eran's comment, to more systematically retry

This adds the retries at the level of the SqlRecoveryDb object
to systematically apply retries to all postgres operations.

* fixup missing "should_retry()" calls

* fix clippy
Copy link

@wjuan-mob wjuan-mob left a comment

Choose a reason for hiding this comment

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

Quick question: Why do we use the retry crate for the db but not the controller, where we use this looping logic with increasing timeout?

@cbeck88
Copy link
Contributor Author

cbeck88 commented Feb 2, 2022

@wjuan-mob the retry crate is not super well designed and usually requires wrapping it to use effectively. The main thing it offers is, the duration iterator thing, and the notion of retriable and non-retriable errors. When we are retrying in an infinite loop and all errors are retried, I think the code is simpler and easier to read if we just use loop and don't try to shoe horn it into retry interface. There were review comments about this and discussion in the master version of this PR

@wjuan-mob
Copy link

@wjuan-mob the retry crate is not super well designed and usually requires wrapping it to use effectively. The main thing it offers is, the duration iterator thing, and the notion of retriable and non-retriable errors. When we are retrying in an infinite loop and all errors are retried, I think the code is simpler and easier to read if we just use loop and don't try to shoe horn it into retry interface. There were review comments about this and discussion in the master version of this PR

Oh yeah. Sorry. I actually did remember reading that thread but I had a brain fart. Thank you.

@cbeck88 cbeck88 merged commit 397d49d into mobilecoinfoundation:release-1.1.3 Feb 4, 2022
@cbeck88 cbeck88 deleted the backport-postgres-retries branch February 4, 2022 21:07
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