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

CASSGO-28 ignore error retry type #1811

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

Conversation

Rikkuru
Copy link

@Rikkuru Rikkuru commented Sep 4, 2024

Closes #1808

@Rikkuru Rikkuru changed the title issue1808: ignore error retry type issue-1808: ignore error retry type Sep 4, 2024
@joao-r-reis
Copy link
Contributor

Can you add a test to ensure that the issue is actually fixed? And to prevent regressions.

@joao-r-reis joao-r-reis changed the title issue-1808: ignore error retry type CASSGO-28 ignore error retry type Nov 5, 2024
@Rikkuru
Copy link
Author

Rikkuru commented Nov 7, 2024

This branch has no conflicts with the base branch when rebasing

Hello! OK I will try to get to it on this weekend

@Rikkuru Rikkuru force-pushed the issue1808 branch 2 times, most recently from 68ccfd0 to 5a5bf22 Compare November 9, 2024 15:08
@Rikkuru
Copy link
Author

Rikkuru commented Nov 9, 2024

Can you add a test to ensure that the issue is actually fixed? And to prevent regressions.

Added a test. Wanted to let observer know that error was ignored but observer is called before retry policy is checked.

there is one more problem - error will not be ignored if we have no more attempts. Added a fix&test for this too

Copy link
Contributor

@joao-r-reis joao-r-reis left a comment

Choose a reason for hiding this comment

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

Looks good. +1 assuming CI passes.

Please format your commit message following the rules stated in CONTRIBUTING.md and add this JIRA to the CHANGELOG.md in the Fixed section of Unreleased, thanks!

If you can't squash and rewrite the commit message the committer will do it for you when the PR is merged.

We need another committer +1 to merge this.

RetryType Ignore is documented and should work according to the documentation. Error is not returned to caller on ignore but can be seen in ObservedQuery. RetryType should be checked even if RetryPolicy.Attempt returns false, otherwise Ignore will not work on last attempt.

Patch by Rikkuru; reviewed by João Reis for CASSGO-28
@Rikkuru
Copy link
Author

Rikkuru commented Nov 18, 2024

We need another committer +1 to merge this.

You mean anyone who've made a commit or apache employee ?

@joao-r-reis
Copy link
Contributor

We need another committer +1 to merge this.

You mean anyone who've made a commit or apache employee ?

Committers are people who have write access to the codebase, a PR needs two +1s from committers to get merged ideally and I'm a committer so we need one more.

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.

CASSGO-28 Retry type Ignore is not different from Rethrow (error is still returned)
2 participants