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

Use latest scylla-rust-driver version - 1.0 #47

Merged
merged 1 commit into from
Mar 14, 2025

Conversation

vponomaryov
Copy link
Collaborator

No description provided.

@vponomaryov
Copy link
Collaborator Author

@vponomaryov vponomaryov requested a review from Copilot March 11, 2025 18:15
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR upgrades the scylla-rust-driver dependency to version 1.0 and updates the code to use the new API modules and types. Key changes include:

  • Updating Cargo.toml to use scylla version 1.0 with new features.
  • Refactoring queries, prepared statements, paging, and error handling in Context and related modules.
  • Adjusting conversions and bindings to work with the new driver API in the bind and connect modules.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/scripting/context.rs Updated session types, query methods, and paging logic.
Cargo.toml Upgraded scylla dependency version and features.
src/scripting/cass_error.rs Adjusted error types and conversion functions.
src/stats/session.rs Modified result handling to match new driver types.
src/scripting/bind.rs Refactored conversion functions for updated types.
src/scripting/connect.rs Updated TLS context creation and session builder usage.
Comments suppressed due to low confidence (1)

src/scripting/context.rs:191

  • Using unwrap() on try_lock() can lead to panics if the lock is unavailable; consider handling the error explicitly to ensure robustness in production.
start_time: TryLock::new(*self.start_time.try_lock().unwrap()),

@vponomaryov vponomaryov force-pushed the scylla-rust-driver-1.0 branch from 790d2be to 189209b Compare March 12, 2025 11:04
@vponomaryov vponomaryov requested a review from Copilot March 12, 2025 11:06
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the project to use the latest scylla-rust-driver version 1.0 and adapts the code accordingly. Key changes include:

  • Migrating from the old driver types and methods (e.g. query → query_unpaged, SSL → TLS) to the new API.
  • Updating error handling and response parsing to match the new driver conventions.
  • Adjusting query parameter binding and column type conversion logic in the binding module.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/scripting/context.rs Updated driver types and query execution methods to the latest API.
Cargo.toml Bumped the scylla dependency version and updated related features.
src/stats/session.rs Adjusted error type imports and response handling in session stats.
src/scripting/connect.rs Replaced SSL context with TLS context as required by the new driver.
src/scripting/cass_error.rs Modified error conversion to use new error types from the updated driver.
src/scripting/bind.rs Updated conversion functions and column type matching to the new API.
Comments suppressed due to low confidence (2)

src/scripting/bind.rs:54

  • Using unwrap on the result of from_f64_retain may panic if the conversion fails. Consider handling the error gracefully instead of unwrapping.
let decimal = rust_decimal::Decimal::from_f64_retain(*v).unwrap();

src/scripting/bind.rs:95

  • Using unwrap on from_str_exact can lead to a panic if the string fails to parse as a decimal. It is recommended to propagate or handle the parsing error.
let decimal = rust_decimal::Decimal::from_str_exact(&dec_str).unwrap();

@vponomaryov vponomaryov requested a review from fruch March 12, 2025 11:11
@fruch fruch requested a review from soyacz March 13, 2025 11:22
Copy link
Collaborator

@fruch fruch left a comment

Choose a reason for hiding this comment

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

LGTM

but I don't know the actual details, it does look like the cql-stress changes

@vponomaryov
Copy link
Collaborator Author

vponomaryov commented Mar 13, 2025

Being tested here: scylla-staging/valerii/vp-longevity-gce-custom-d1-workload2-hybrid-raid#62

The test job passed ok from the latte point of view.
There were different retries, all the unique different queries (single-queries, batch queries, reads, writes, UDTs, Lists, Maps ... ) were working ok.
So, it is ready for merge.

Copy link

@soyacz soyacz left a comment

Choose a reason for hiding this comment

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

I suppose it's ok, I'm not sure if we need to always clone responses - maybe could be removed in some places?

rows_cnt += page.rows.as_ref().map(|r| r.len()).unwrap_or(0) as u64;
Ok((ref page, ref paging_state_response)) => {
rows_cnt += page
.clone()
Copy link

Choose a reason for hiding this comment

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

do we need to clone() here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is rust:

error[E0507]: cannot move out of `*page` which is behind a shared reference
   --> src/scripting/context.rs:578:41
    |
578 | ...                   rows_cnt += page
    |                                   ^^^^ move occurs because `*page` has type `scylla::response::query_result::QueryResult`, which does not implement the `Copy` trait
579 | ...                       // .clone()
580 | ...                       .into_rows_result()
    |                            ------------------ `*page` moved due to this method call
    |
note: `scylla::response::query_result::QueryResult::into_rows_result` takes ownership of the receiver `self`, which moves `*page`
   --> /home/kiparis/.cargo/registry/src/index.crates.io-6f17d22bba15001f/scylla-1.0.0/src/response/query_result.rs:176:29
    |
176 |     pub fn into_rows_result(self) -> Result<QueryRowsResult, IntoRowsResultError> {
    |                             ^^^^
help: you can `clone` the value and consume it, but this might not be your desired behavior
    |
578 |                             rows_cnt += <scylla::response::query_result::QueryResult as Clone>::clone(&page)
    |                                         +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++    +
help: consider cloning the value if the performance cost is acceptable
    |
578 |                             rows_cnt += page.clone()
    |                                             ++++++++

@vponomaryov vponomaryov force-pushed the scylla-rust-driver-1.0 branch from 189209b to 35c0663 Compare March 13, 2025 13:45
@vponomaryov
Copy link
Collaborator Author

@soyacz this one is waiting for you

Copy link

@soyacz soyacz left a comment

Choose a reason for hiding this comment

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

I'm not familiar with latte code base and still beginner with Rust - I'm not sure if my opinion counts thou.
But if it works and didn't degrade performance too much (if degraded) - then LGTM

@vponomaryov vponomaryov merged commit 2b67955 into main Mar 14, 2025
4 checks passed
@vponomaryov vponomaryov deleted the scylla-rust-driver-1.0 branch March 14, 2025 10:45
@roydahan
Copy link

Good job!
Do we want to contribute it back to the upstream?

@vponomaryov
Copy link
Collaborator Author

Good job! Do we want to contribute it back to the upstream?

It won't apply as is. We already have differences.
But yes, it can be done if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants