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.

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

Return Query Error for non existent DC #825

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
review feedback
  • Loading branch information
samuelorji committed Oct 17, 2023
commit f860086c7c28db71c610c6185ffd5b2c4e8900df
14 changes: 7 additions & 7 deletions scylla-cql/src/errors.rs
Original file line number Diff line number Diff line change
@@ -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<QueryError> 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,
}
}
}
2 changes: 1 addition & 1 deletion scylla/src/transport/load_balancing/default.rs
Original file line number Diff line number Diff line change
@@ -2414,7 +2414,7 @@ mod latency_awareness {
| QueryError::IoError(_)
| QueryError::ProtocolError(_)
| QueryError::TimeoutError
| QueryError::NoKnownNodeFoundError(_)
| QueryError::EmptyQueryPlan
| QueryError::RequestTimeout(_) => true,
}
}
12 changes: 8 additions & 4 deletions scylla/src/transport/session.rs
Original file line number Diff line number Diff line change
@@ -1661,15 +1661,15 @@ impl Session {
QueryFut: Future<Output = Result<ResT, QueryError>>,
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<QueryError> = Some(QueryError::NoKnownNodeFoundError(
"Please confirm the supplied datacenters exists".to_string(),
));
let mut last_error: Option<QueryError> = None;
let mut current_consistency: Consistency = context
.consistency_set_on_statement
.unwrap_or(execution_profile.consistency);

let mut query_plan_is_empty = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wanted to do a peekable check to ensure the iterator is not empty like:

query_plan.peekable()

but it caused this lifetime error:

error: higher-ranked lifetime error
   --> scylla/src/transport/session_test.rs:409:5
    |
409 | /     tokio::spawn(async move {
410 | |         let values = (
411 | |             (1_i32, 2_i32, "abc"),
412 | |             (),
...   |
415 | |         session_clone.batch(&batch, values).await.unwrap();
416 | |     })
    | |______^
    |
    = note: could not prove `[async block@scylla/src/transport/session_test.rs:409:18: 416:6]: std::marker::Send`
    ```
    So, I just defaulted to doing this more simple check


'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)
}

5 changes: 1 addition & 4 deletions scylla/src/transport/session_test.rs
Original file line number Diff line number Diff line change
@@ -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)
}