Skip to content

Commit

Permalink
retry_policy: replace query mentions with request
Browse files Browse the repository at this point in the history
  • Loading branch information
@muzarski
muzarski committed Jan 15, 2025
1 parent b94f6b5 commit 5485130
Show file tree
Hide file tree
Showing 9 changed files with 100 additions and 100 deletions.
4 changes: 2 additions & 2 deletions scylla/src/client/pager.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ use crate::observability::driver_tracing::RequestSpan;
use crate::observability::history::{self, HistoryListener};
use crate::observability::metrics::Metrics;
use crate::policies::load_balancing::{self, RoutingInfo};
use crate::policies::retry::{QueryInfo, RetryDecision, RetrySession};
use crate::policies::retry::{RequestInfo, RetryDecision, RetrySession};
use crate::response::query_result::ColumnSpecs;
use crate::response::{NonErrorQueryResponse, QueryResponse};
use crate::statement::{prepared_statement::PreparedStatement, query::Query};
Expand Down Expand Up @@ -223,7 +223,7 @@ where
};

// Use retry policy to decide what to do next
let query_info = QueryInfo {
let query_info = RequestInfo {
error: &request_error,
is_idempotent: self.query_is_idempotent,
consistency: self.query_consistency,
Expand Down
4 changes: 2 additions & 2 deletions scylla/src/client/session.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use crate::observability::tracing::TracingInfo;
use crate::policies::address_translator::AddressTranslator;
use crate::policies::host_filter::HostFilter;
use crate::policies::load_balancing::{self, RoutingInfo};
use crate::policies::retry::{QueryInfo, RetryDecision, RetrySession};
use crate::policies::retry::{RequestInfo, RetryDecision, RetrySession};
use crate::policies::speculative_execution;
use crate::prepared_statement::PreparedStatement;
use crate::query::Query;
Expand Down Expand Up @@ -2085,7 +2085,7 @@ where
};

// Use retry policy to decide what to do next
let query_info = QueryInfo {
let query_info = RequestInfo {
error: &request_error,
is_idempotent: context.is_idempotent,
consistency: context
Expand Down
4 changes: 2 additions & 2 deletions scylla/src/client/session_test.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use crate::cluster::metadata::{CollectionType, ColumnKind, CqlType, NativeType,
use crate::deserialize::DeserializeOwnedValue;
use crate::errors::{BadKeyspaceName, BadQuery, DbError, QueryError};
use crate::observability::tracing::TracingInfo;
use crate::policies::retry::{QueryInfo, RetryDecision, RetryPolicy, RetrySession};
use crate::policies::retry::{RequestInfo, RetryDecision, RetryPolicy, RetrySession};
use crate::prepared_statement::PreparedStatement;
use crate::query::Query;
use crate::routing::partitioner::{
Expand Down Expand Up @@ -2725,7 +2725,7 @@ async fn test_iter_works_when_retry_policy_returns_ignore_write_error() {

struct MyRetrySession(Arc<AtomicBool>);
impl RetrySession for MyRetrySession {
fn decide_should_retry(&mut self, _: QueryInfo) -> RetryDecision {
fn decide_should_retry(&mut self, _: RequestInfo) -> RetryDecision {
self.0.store(true, Ordering::Relaxed);
RetryDecision::IgnoreWriteError
}
Expand Down
68 changes: 34 additions & 34 deletions scylla/src/policies/retry/default.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use scylla_cql::frame::response::error::{DbError, WriteType};

use crate::errors::RequestAttemptError;

use super::{QueryInfo, RetryDecision, RetryPolicy, RetrySession};
use super::{RequestInfo, RetryDecision, RetryPolicy, RetrySession};

/// Default retry policy - retries when there is a high chance that a retry might help.\
/// Behaviour based on [DataStax Java Driver](https://docs.datastax.com/en/developer/java-driver/4.10/manual/core/retries/)
Expand Down Expand Up @@ -50,18 +50,18 @@ impl Default for DefaultRetrySession {
}

impl RetrySession for DefaultRetrySession {
fn decide_should_retry(&mut self, query_info: QueryInfo) -> RetryDecision {
if query_info.consistency.is_serial() {
fn decide_should_retry(&mut self, request_info: RequestInfo) -> RetryDecision {
if request_info.consistency.is_serial() {
return RetryDecision::DontRetry;
};
match query_info.error {
match request_info.error {
// Basic errors - there are some problems on this node
// Retry on a different one if possible
RequestAttemptError::BrokenConnectionError(_)
| RequestAttemptError::DbError(DbError::Overloaded, _)
| RequestAttemptError::DbError(DbError::ServerError, _)
| RequestAttemptError::DbError(DbError::TruncateError, _) => {
if query_info.is_idempotent {
if request_info.is_idempotent {
RetryDecision::RetryNextNode(None)
} else {
RetryDecision::DontRetry
Expand Down Expand Up @@ -108,7 +108,7 @@ impl RetrySession for DefaultRetrySession {
// By the time we retry they should be detected as dead.
RequestAttemptError::DbError(DbError::WriteTimeout { write_type, .. }, _) => {
if !self.was_write_timeout_retry
&& query_info.is_idempotent
&& request_info.is_idempotent
&& *write_type == WriteType::BatchLog
{
self.was_write_timeout_retry = true;
Expand All @@ -117,7 +117,7 @@ impl RetrySession for DefaultRetrySession {
RetryDecision::DontRetry
}
}
// The node is still bootstrapping it can't execute the query, we should try another one
// The node is still bootstrapping it can't execute the request, we should try another one
RequestAttemptError::DbError(DbError::IsBootstrapping, _) => {
RetryDecision::RetryNextNode(None)
}
Expand All @@ -135,16 +135,16 @@ impl RetrySession for DefaultRetrySession {

#[cfg(test)]
mod tests {
use super::{DefaultRetryPolicy, QueryInfo, RetryDecision, RetryPolicy};
use super::{DefaultRetryPolicy, RequestInfo, RetryDecision, RetryPolicy};
use crate::errors::{BrokenConnectionErrorKind, RequestAttemptError};
use crate::errors::{DbError, WriteType};
use crate::statement::Consistency;
use crate::test_utils::setup_tracing;
use bytes::Bytes;
use scylla_cql::frame::frame_errors::{BatchSerializationError, CqlRequestSerializationError};

fn make_query_info(error: &RequestAttemptError, is_idempotent: bool) -> QueryInfo<'_> {
QueryInfo {
fn make_request_info(error: &RequestAttemptError, is_idempotent: bool) -> RequestInfo<'_> {
RequestInfo {
error,
is_idempotent,
consistency: Consistency::One,
Expand All @@ -155,13 +155,13 @@ mod tests {
fn default_policy_assert_never_retries(error: RequestAttemptError) {
let mut policy = DefaultRetryPolicy::new().new_session();
assert_eq!(
policy.decide_should_retry(make_query_info(&error, false)),
policy.decide_should_retry(make_request_info(&error, false)),
RetryDecision::DontRetry
);

let mut policy = DefaultRetryPolicy::new().new_session();
assert_eq!(
policy.decide_should_retry(make_query_info(&error, true)),
policy.decide_should_retry(make_request_info(&error, true)),
RetryDecision::DontRetry
);
}
Expand Down Expand Up @@ -229,13 +229,13 @@ mod tests {
fn default_policy_assert_idempotent_next(error: RequestAttemptError) {
let mut policy = DefaultRetryPolicy::new().new_session();
assert_eq!(
policy.decide_should_retry(make_query_info(&error, false)),
policy.decide_should_retry(make_request_info(&error, false)),
RetryDecision::DontRetry
);

let mut policy = DefaultRetryPolicy::new().new_session();
assert_eq!(
policy.decide_should_retry(make_query_info(&error, true)),
policy.decide_should_retry(make_request_info(&error, true)),
RetryDecision::RetryNextNode(None)
);
}
Expand Down Expand Up @@ -265,13 +265,13 @@ mod tests {

let mut policy = DefaultRetryPolicy::new().new_session();
assert_eq!(
policy.decide_should_retry(make_query_info(&error, false)),
policy.decide_should_retry(make_request_info(&error, false)),
RetryDecision::RetryNextNode(None)
);

let mut policy = DefaultRetryPolicy::new().new_session();
assert_eq!(
policy.decide_should_retry(make_query_info(&error, true)),
policy.decide_should_retry(make_request_info(&error, true)),
RetryDecision::RetryNextNode(None)
);
}
Expand All @@ -291,21 +291,21 @@ mod tests {

let mut policy_not_idempotent = DefaultRetryPolicy::new().new_session();
assert_eq!(
policy_not_idempotent.decide_should_retry(make_query_info(&error, false)),
policy_not_idempotent.decide_should_retry(make_request_info(&error, false)),
RetryDecision::RetryNextNode(None)
);
assert_eq!(
policy_not_idempotent.decide_should_retry(make_query_info(&error, false)),
policy_not_idempotent.decide_should_retry(make_request_info(&error, false)),
RetryDecision::DontRetry
);

let mut policy_idempotent = DefaultRetryPolicy::new().new_session();
assert_eq!(
policy_idempotent.decide_should_retry(make_query_info(&error, true)),
policy_idempotent.decide_should_retry(make_request_info(&error, true)),
RetryDecision::RetryNextNode(None)
);
assert_eq!(
policy_idempotent.decide_should_retry(make_query_info(&error, true)),
policy_idempotent.decide_should_retry(make_request_info(&error, true)),
RetryDecision::DontRetry
);
}
Expand All @@ -328,22 +328,22 @@ mod tests {
// Not idempotent
let mut policy = DefaultRetryPolicy::new().new_session();
assert_eq!(
policy.decide_should_retry(make_query_info(&enough_responses_no_data, false)),
policy.decide_should_retry(make_request_info(&enough_responses_no_data, false)),
RetryDecision::RetrySameNode(None)
);
assert_eq!(
policy.decide_should_retry(make_query_info(&enough_responses_no_data, false)),
policy.decide_should_retry(make_request_info(&enough_responses_no_data, false)),
RetryDecision::DontRetry
);

// Idempotent
let mut policy = DefaultRetryPolicy::new().new_session();
assert_eq!(
policy.decide_should_retry(make_query_info(&enough_responses_no_data, true)),
policy.decide_should_retry(make_request_info(&enough_responses_no_data, true)),
RetryDecision::RetrySameNode(None)
);
assert_eq!(
policy.decide_should_retry(make_query_info(&enough_responses_no_data, true)),
policy.decide_should_retry(make_request_info(&enough_responses_no_data, true)),
RetryDecision::DontRetry
);

Expand All @@ -362,14 +362,14 @@ mod tests {
// Not idempotent
let mut policy = DefaultRetryPolicy::new().new_session();
assert_eq!(
policy.decide_should_retry(make_query_info(&enough_responses_with_data, false)),
policy.decide_should_retry(make_request_info(&enough_responses_with_data, false)),
RetryDecision::DontRetry
);

// Idempotent
let mut policy = DefaultRetryPolicy::new().new_session();
assert_eq!(
policy.decide_should_retry(make_query_info(&enough_responses_with_data, true)),
policy.decide_should_retry(make_request_info(&enough_responses_with_data, true)),
RetryDecision::DontRetry
);

Expand All @@ -387,19 +387,19 @@ mod tests {
// Not idempotent
let mut policy = DefaultRetryPolicy::new().new_session();
assert_eq!(
policy.decide_should_retry(make_query_info(&not_enough_responses_with_data, false)),
policy.decide_should_retry(make_request_info(&not_enough_responses_with_data, false)),
RetryDecision::DontRetry
);

// Idempotent
let mut policy = DefaultRetryPolicy::new().new_session();
assert_eq!(
policy.decide_should_retry(make_query_info(&not_enough_responses_with_data, true)),
policy.decide_should_retry(make_request_info(&not_enough_responses_with_data, true)),
RetryDecision::DontRetry
);
}

// WriteTimeout will retry once when the query is idempotent and write_type == BatchLog
// WriteTimeout will retry once when the request is idempotent and write_type == BatchLog
#[test]
fn default_write_timeout() {
setup_tracing();
Expand All @@ -417,18 +417,18 @@ mod tests {
// Not idempotent
let mut policy = DefaultRetryPolicy::new().new_session();
assert_eq!(
policy.decide_should_retry(make_query_info(&good_write_type, false)),
policy.decide_should_retry(make_request_info(&good_write_type, false)),
RetryDecision::DontRetry
);

// Idempotent
let mut policy = DefaultRetryPolicy::new().new_session();
assert_eq!(
policy.decide_should_retry(make_query_info(&good_write_type, true)),
policy.decide_should_retry(make_request_info(&good_write_type, true)),
RetryDecision::RetrySameNode(None)
);
assert_eq!(
policy.decide_should_retry(make_query_info(&good_write_type, true)),
policy.decide_should_retry(make_request_info(&good_write_type, true)),
RetryDecision::DontRetry
);

Expand All @@ -446,14 +446,14 @@ mod tests {
// Not idempotent
let mut policy = DefaultRetryPolicy::new().new_session();
assert_eq!(
policy.decide_should_retry(make_query_info(&bad_write_type, false)),
policy.decide_should_retry(make_request_info(&bad_write_type, false)),
RetryDecision::DontRetry
);

// Idempotent
let mut policy = DefaultRetryPolicy::new().new_session();
assert_eq!(
policy.decide_should_retry(make_query_info(&bad_write_type, true)),
policy.decide_should_retry(make_request_info(&bad_write_type, true)),
RetryDecision::DontRetry
);
}
Expand Down
Loading

0 comments on commit 5485130

Please sign in to comment.