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

feat: more automatic retries #43

Merged
merged 2 commits into from
Jan 11, 2022
Merged

feat: more automatic retries #43

merged 2 commits into from
Jan 11, 2022

Conversation

tustvold
Copy link
Contributor

Configures automatic handling of IO and Poisoned errors

})
.await
.map_err(Error::Metadata)?;
let mut backoff = Backoff::new(&self.backoff_config);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially played around with extracting this retry logic so that it could be shared between here and the PartitionClient, but the different error types made this complicated and so I opted for the simple solution

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could add a trait for the errors:

trait ErrorHandling {
    fn should_invalidate_broker() -> bool;
}

impl ErrorHandling for RequestError {...}
...

If I understand the logic correctly this is all you currently need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You also need to know what types of errors can be handled , which varies because they are different error enumerations...

I also experimented with just using a single error enumeration for all the clients, this actually works quite nicely but was a substantial amount of churn and I got fed up 😆

Copy link
Collaborator

@crepererum crepererum left a comment

Choose a reason for hiding this comment

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

I'm a bit worried that we have no real tests for this.

@tustvold
Copy link
Contributor Author

Created #49 to track improving the test coverage, I agree it should be tested

@tustvold tustvold added automerge Instruct kodiak to merge the PR kodiak: merge.method = 'squash' Instruct kodiak to perform a squash merge labels Jan 11, 2022
@kodiakhq kodiakhq bot merged commit 44765c9 into main Jan 11, 2022
@carols10cents carols10cents deleted the retry-handling branch January 21, 2022 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Instruct kodiak to merge the PR kodiak: merge.method = 'squash' Instruct kodiak to perform a squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants