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.

Sign up for GitHub

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

Jump to bottom

CASSGO-27 fix gocql-RetryPolicy-still-dont-use-query-idempotence #1809

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

CASSGO-27 fix gocql-RetryPolicy-still-dont-use-query-idempotence #1809

wants to merge 1 commit into from

Conversation

OleksiienkoMykyta
Copy link

Closes #1803
RetryPolicy doesn't check the query's idempotency, but according to the specification queries with false idempotence shouldn't be retried.

@SiyaoIsHiding
Copy link

Hi, I think this won't fix the issue because of this line

cassandra-gocql-driver/query_executor.go

Lines 67 to 69 in 92b4056

if !qry.IsIdempotent() || sp.Attempts() == 0 {
return q.do(qry.Context(), qry, hostIter), nil
}

the function do is only called when it's marked as NOT idempotent.

@joao-r-reis
Copy link
Contributor

@SiyaoIsHiding that line you linked is to avoid speculative executions if the query isn't idempotent which seems correct to me... It's not really related to retries. If it's idempotent then run will be called which will then call do

@joao-r-reis
Copy link
Contributor

joao-r-reis commented Nov 5, 2024 •
edited
Loading

@OleksiienkoMykyta can you add a test that reproduces the bug before this change to ensure we don't get a regression in the future? It should be possible to reproduce this by using a very small timeout

@joao-r-reis joao-r-reis changed the title issue-1803, fix gocql-RetryPolicy-still-dont-use-query-idempotence CASSGO-27 fix gocql-RetryPolicy-still-dont-use-query-idempotence Nov 5, 2024
@OleksiienkoMykyta
Copy link
Author

@OleksiienkoMykyta can you add a test that reproduces the bug before this change to ensure we don't get a regression in the future? It should be possible to reproduce this by using a very small timeout.

Yes, sure, I'll add some tests to cover this behavior.

@OleksiienkoMykyta
Copy link
Author

@joao-r-reis I have added some tests, it would be nice if you would check it.
Also, I tried to test it with a timeout, but it doesn't reconcile query execution, just a single try, so I used a custom RetryPolicy.

@joao-r-reis
Copy link
Contributor

TestQueryMultinodeWithMetrics is panicking because it assumes the retry policy runs for all hosts but now it doesn't because of the idempotency change.

joao-r-reis
joao-r-reis reviewed Nov 7, 2024
View reviewed changes
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.

Changes look good 👍 , still need to fix failing tests though.

@OleksiienkoMykyta OleksiienkoMykyta force-pushed the gocql-RetryPolicy-still-dont-use-query-idempotence branch from 9ce97bb to 072a04e Compare November 8, 2024 11:38
@OleksiienkoMykyta
Copy link
Author

Changes look good 👍 , still need to fix failing tests though.

I have fixed it. The issue was in test which should make retries but doesn't set idempotence to query explicitly, now everything works properly.

@joao-r-reis
Copy link
Contributor

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!

We need another committer +1 to merge this.

@OleksiienkoMykyta
CASSGO-27 gocql RetryPolicy dont use query idempotence
a262096 
RetryPolicy doesn't check the query's idempotency,
but according to the specification queries with false idempotence shouldn't be retried.

patch by Mykyta Oleksiienko; reviewed by João Reis for CASSGO-27
@OleksiienkoMykyta OleksiienkoMykyta force-pushed the gocql-RetryPolicy-still-dont-use-query-idempotence branch from 072a04e to a262096 Compare November 13, 2024 12:40
jfleming-ic
jfleming-ic reviewed Nov 18, 2024
View reviewed changes
CHANGELOG.md
@@ -14,6 +14,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Fixed

- CASSGO-27
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to have a short summary of what was fixed here, so users don't need to lookup what this ticket is, convention seems to be
"message" (ticket)

So for this, something like this would suffice

"Retry policy now takes into account query idempotency (CASSGO-27)"

@Jacko161
Copy link

Sorry was on the wrong github account on my last comment - happy to tick this, but would be good to update the changelog message if possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfleming-ic jfleming-ic jfleming-ic left review comments

@joao-r-reis joao-r-reis joao-r-reis approved these changes

@Jacko161 Jacko161 Jacko161 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CASSGO-27 gocql RetryPolicies still don't use query idempotence
5 participants
@OleksiienkoMykyta @SiyaoIsHiding @joao-r-reis @Jacko161 @jfleming-ic