From f631df76d52a161ad7a86509287954613a4f5183 Mon Sep 17 00:00:00 2001 From: samuel orji Date: Sun, 8 Oct 2023 19:53:37 +0100 Subject: [PATCH 1/3] create a new query error called NoKnownNodeFoundError that is returned when there are no known nodes found during query plan and avoid ambiguity --- scylla-cql/src/errors.rs | 9 +++++ .../src/transport/load_balancing/default.rs | 1 + scylla/src/transport/session.rs | 5 ++- scylla/src/transport/session_test.rs | 34 +++++++++++++++++++ 4 files changed, 48 insertions(+), 1 deletion(-) diff --git a/scylla-cql/src/errors.rs b/scylla-cql/src/errors.rs index 9e80247e20..957696ed1d 100644 --- a/scylla-cql/src/errors.rs +++ b/scylla-cql/src/errors.rs @@ -46,6 +46,10 @@ pub enum QueryError { #[error("Request timeout: {0}")] RequestTimeout(String), + /// No known node found to perform query + #[error("No known node found: {0}")] + NoKnownNodeFoundError(String), + /// Address translation failed #[error("Address translation failed: {0}")] TranslationError(#[from] TranslationError), @@ -404,6 +408,10 @@ pub enum NewSessionError { #[error("Client timeout: {0}")] RequestTimeout(String), + /// No known node found to perform query + #[error("No known node found: {0}")] + NoKnownNodeFoundError(String), + /// Address translation failed #[error("Address translation failed: {0}")] TranslationError(#[from] TranslationError), @@ -482,6 +490,7 @@ impl From for NewSessionError { QueryError::UnableToAllocStreamId => NewSessionError::UnableToAllocStreamId, QueryError::RequestTimeout(msg) => NewSessionError::RequestTimeout(msg), QueryError::TranslationError(e) => NewSessionError::TranslationError(e), + QueryError::NoKnownNodeFoundError(e) => NewSessionError::NoKnownNodeFoundError(e), } } } diff --git a/scylla/src/transport/load_balancing/default.rs b/scylla/src/transport/load_balancing/default.rs index d1554babf1..e125f0ce34 100644 --- a/scylla/src/transport/load_balancing/default.rs +++ b/scylla/src/transport/load_balancing/default.rs @@ -2414,6 +2414,7 @@ mod latency_awareness { | QueryError::IoError(_) | QueryError::ProtocolError(_) | QueryError::TimeoutError + | QueryError::NoKnownNodeFoundError(_) | QueryError::RequestTimeout(_) => true, } } diff --git a/scylla/src/transport/session.rs b/scylla/src/transport/session.rs index 2f67874f8c..cf66fda436 100644 --- a/scylla/src/transport/session.rs +++ b/scylla/src/transport/session.rs @@ -1661,7 +1661,10 @@ impl Session { QueryFut: Future>, ResT: AllowedRunQueryResTType, { - let mut last_error: Option = None; + // set default error as no known found as the query plan returns an empty iterator if there are no nodes in the plan + let mut last_error: Option = Some(QueryError::NoKnownNodeFoundError( + "Please confirm the supplied datacenters exists".to_string(), + )); let mut current_consistency: Consistency = context .consistency_set_on_statement .unwrap_or(execution_profile.consistency); diff --git a/scylla/src/transport/session_test.rs b/scylla/src/transport/session_test.rs index 805217053d..e2e7fc14b3 100644 --- a/scylla/src/transport/session_test.rs +++ b/scylla/src/transport/session_test.rs @@ -2,6 +2,7 @@ use crate as scylla; use crate::batch::{Batch, BatchStatement}; use crate::frame::response::result::Row; use crate::frame::value::ValueList; +use crate::load_balancing::DefaultPolicy; use crate::prepared_statement::PreparedStatement; use crate::query::Query; use crate::retry_policy::{QueryInfo, RetryDecision, RetryPolicy, RetrySession}; @@ -2857,3 +2858,36 @@ async fn test_manual_primary_key_computation() { .await; } } + +#[tokio::test] +async fn test_non_existent_dc_return_correct_error() { + let ks = "iot"; + + let host = "127.0.0.1"; + let dc = "non existent dc"; + + let default_policy = DefaultPolicy::builder() + .prefer_datacenter(dc.to_string()) + .build(); + + let profile = ExecutionProfile::builder() + .load_balancing_policy(default_policy) + .build(); + + let handle = profile.into_handle(); + + let session: Session = SessionBuilder::new() + .known_node(host) + .default_execution_profile_handle(handle) + .build() + .await + .expect("cannot create session"); + + let ks_stmt = format!("CREATE KEYSPACE IF NOT EXISTS {} WITH replication = {{'class': 'NetworkTopologyStrategy', '{}': 1}}", ks, dc); + let query_result = session.query(ks_stmt, &[]).await; + + assert_matches!( + query_result.unwrap_err(), + QueryError::NoKnownNodeFoundError(_) + ) +} From f860086c7c28db71c610c6185ffd5b2c4e8900df Mon Sep 17 00:00:00 2001 From: samuel orji Date: Tue, 17 Oct 2023 16:08:20 +0100 Subject: [PATCH 2/3] review feedback --- scylla-cql/src/errors.rs | 14 +++++++------- scylla/src/transport/load_balancing/default.rs | 2 +- scylla/src/transport/session.rs | 12 ++++++++---- scylla/src/transport/session_test.rs | 5 +---- 4 files changed, 17 insertions(+), 16 deletions(-) diff --git a/scylla-cql/src/errors.rs b/scylla-cql/src/errors.rs index 957696ed1d..b93d4520db 100644 --- a/scylla-cql/src/errors.rs +++ b/scylla-cql/src/errors.rs @@ -46,9 +46,9 @@ pub enum QueryError { #[error("Request timeout: {0}")] RequestTimeout(String), - /// No known node found to perform query - #[error("No known node found: {0}")] - NoKnownNodeFoundError(String), + /// Empty Query Plan + #[error("Load balancing policy returned empty query plan. It can happen when the driver is provided with non-existing datacenter name")] + EmptyQueryPlan, /// Address translation failed #[error("Address translation failed: {0}")] @@ -408,9 +408,9 @@ pub enum NewSessionError { #[error("Client timeout: {0}")] RequestTimeout(String), - /// No known node found to perform query - #[error("No known node found: {0}")] - NoKnownNodeFoundError(String), + /// Empty Query Plan + #[error("Load balancing policy returned empty query plan. It can happen when the driver is provided with non-existing datacenter name")] + EmptyQueryPlan, /// Address translation failed #[error("Address translation failed: {0}")] @@ -490,7 +490,7 @@ impl From for NewSessionError { QueryError::UnableToAllocStreamId => NewSessionError::UnableToAllocStreamId, QueryError::RequestTimeout(msg) => NewSessionError::RequestTimeout(msg), QueryError::TranslationError(e) => NewSessionError::TranslationError(e), - QueryError::NoKnownNodeFoundError(e) => NewSessionError::NoKnownNodeFoundError(e), + QueryError::EmptyQueryPlan => NewSessionError::EmptyQueryPlan, } } } diff --git a/scylla/src/transport/load_balancing/default.rs b/scylla/src/transport/load_balancing/default.rs index e125f0ce34..a915890c26 100644 --- a/scylla/src/transport/load_balancing/default.rs +++ b/scylla/src/transport/load_balancing/default.rs @@ -2414,7 +2414,7 @@ mod latency_awareness { | QueryError::IoError(_) | QueryError::ProtocolError(_) | QueryError::TimeoutError - | QueryError::NoKnownNodeFoundError(_) + | QueryError::EmptyQueryPlan | QueryError::RequestTimeout(_) => true, } } diff --git a/scylla/src/transport/session.rs b/scylla/src/transport/session.rs index cf66fda436..5a8670018b 100644 --- a/scylla/src/transport/session.rs +++ b/scylla/src/transport/session.rs @@ -1661,15 +1661,15 @@ impl Session { QueryFut: Future>, ResT: AllowedRunQueryResTType, { - // set default error as no known found as the query plan returns an empty iterator if there are no nodes in the plan - let mut last_error: Option = Some(QueryError::NoKnownNodeFoundError( - "Please confirm the supplied datacenters exists".to_string(), - )); + let mut last_error: Option = None; let mut current_consistency: Consistency = context .consistency_set_on_statement .unwrap_or(execution_profile.consistency); + let mut query_plan_is_empty = true; + 'nodes_in_plan: for node in query_plan { + query_plan_is_empty = false; let span = trace_span!("Executing query", node = %node.address); 'same_node_retries: loop { trace!(parent: &span, "Execution started"); @@ -1772,6 +1772,10 @@ impl Session { } } + if query_plan_is_empty { + return Some(Err(QueryError::EmptyQueryPlan)); + } + last_error.map(Result::Err) } diff --git a/scylla/src/transport/session_test.rs b/scylla/src/transport/session_test.rs index e2e7fc14b3..dcf8dce03e 100644 --- a/scylla/src/transport/session_test.rs +++ b/scylla/src/transport/session_test.rs @@ -2886,8 +2886,5 @@ async fn test_non_existent_dc_return_correct_error() { let ks_stmt = format!("CREATE KEYSPACE IF NOT EXISTS {} WITH replication = {{'class': 'NetworkTopologyStrategy', '{}': 1}}", ks, dc); let query_result = session.query(ks_stmt, &[]).await; - assert_matches!( - query_result.unwrap_err(), - QueryError::NoKnownNodeFoundError(_) - ) + assert_matches!(query_result.unwrap_err(), QueryError::EmptyQueryPlan) } From 2e8f14e6d9e0cf98b1a821885795101e31be88b2 Mon Sep 17 00:00:00 2001 From: samuel orji Date: Thu, 19 Oct 2023 14:15:03 +0100 Subject: [PATCH 3/3] review feedback: use unique keyspace and generic create session builder --- scylla/src/transport/session_test.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/scylla/src/transport/session_test.rs b/scylla/src/transport/session_test.rs index dcf8dce03e..f26fb73bc4 100644 --- a/scylla/src/transport/session_test.rs +++ b/scylla/src/transport/session_test.rs @@ -2861,11 +2861,9 @@ async fn test_manual_primary_key_computation() { #[tokio::test] async fn test_non_existent_dc_return_correct_error() { - let ks = "iot"; + let ks = unique_keyspace_name(); - let host = "127.0.0.1"; let dc = "non existent dc"; - let default_policy = DefaultPolicy::builder() .prefer_datacenter(dc.to_string()) .build(); @@ -2876,14 +2874,13 @@ async fn test_non_existent_dc_return_correct_error() { let handle = profile.into_handle(); - let session: Session = SessionBuilder::new() - .known_node(host) + let session: Session = create_new_session_builder() .default_execution_profile_handle(handle) .build() .await .expect("cannot create session"); - let ks_stmt = format!("CREATE KEYSPACE IF NOT EXISTS {} WITH replication = {{'class': 'NetworkTopologyStrategy', '{}': 1}}", ks, dc); + let ks_stmt = format!("CREATE KEYSPACE IF NOT EXISTS {} WITH replication = {{'class': 'NetworkTopologyStrategy', 'replication_factor' : 1}}", ks); let query_result = session.query(ks_stmt, &[]).await; assert_matches!(query_result.unwrap_err(), QueryError::EmptyQueryPlan)