Honor the idempotent flag for retries #346
Conversation
|
|
||
| // 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 | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
maybe, I am not sure, based on @pdbossman issue I thought that retires should not work for idempotent queries in general
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Well, reason to this change is this:
- In some cases cluster can return failure, but query could be executed, when we retry in such case, we effectively execute it twice
- 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:
- Identify errors that signal a case when query could be executed, despite the error.
- Void to retry in these cases for idepotent query.
List of these errors (not complete):
- Timeout from both driver and server.
- 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.
|
This PR is not correct, and we will need some refactor to actually solve idempotent queries retries, more on that in the issue #331 |
Fixes: #331