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

Honor the idempotent flag for retries #346

Closed
wants to merge 1 commit into from
Closed

Honor the idempotent flag for retries #346

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion query_executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,8 @@ func (q *queryExecutor) do(ctx context.Context, qry ExecutableQuery, hostIter Ne

// Exit if the query was successful
// or no retry policy defined
if iter.err == nil || rt == nil {
// or query is not marked as idempotent
if iter.err == nil || rt == nil || !qry.IsIdempotent() {
return iter
}
Comment on lines 149 to 155
Copy link

Choose a reason for hiding this comment

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

Shouldn't this depend on the error returned? For some it should be fine to retry non-idempotent queries. Examples:

  • Overloaded (scylla-specific)
  • DbError::Unavailable
  • DbError::IsBootstrapping

Those examples were taken from an open PR by @muzarski to Rust Driver: https://github.com/scylladb/scylla-rust-driver/pull/1083/files#diff-190ddd8a3bf1cb700a17e2fa417c86f5bcec57edaacc62e8369e96383c9958c4

This PR is not final - we will be discussing how to handle each type of error, any one is of course invited to participate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe, I am not sure, based on @pdbossman issue I thought that retires should not work for idempotent queries in general

Copy link

Choose a reason for hiding this comment

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

If the query is not idempotent, then it is not safe to execute it multiple times. If we don't know if the query was executed, then we can't execute it again - because if it was executed the first time, then we have second execution which is not correct.

However, if we know that the query was not executed, then it is fine to execute it again (= it is fine to retry). Some errors (for example the ones I mentioned above) imply that the query was not executed, so they should not prevent retry, even for non-idempotent queries.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, reason to this change is this:

  1. In some cases cluster can return failure, but query could be executed, when we retry in such case, we effectively execute it twice
  2. Indempotent queries are queries that potentially or practically will produce different result if being executed twice, so we should avoid it from happening.

Now, way to do that properly:

  1. Identify errors that signal a case when query could be executed, despite the error.
  2. Void to retry in these cases for idepotent query.

List of these errors (not complete):

  1. Timeout from both driver and server.
  2. Any error after request was sent, but response wasn't yet received.

Also it is better to make it so that final error returned has distinctive type, so that user could distinct cases when to check if data is there.


Expand Down
Loading