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.

Sign up for GitHub

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

Fix/batch length #824

Merged
merged 7 commits into from
Oct 17, 2023
+104 −22
Merged

Fix/batch length #824

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4c013e8
changed the type of the maximum number of statements in a batch query…
samuelorji Oct 6, 2023
772c46f
add a guard when doing batch statements to prevent making calls to th…
samuelorji Oct 6, 2023
efb96fc
review feedback: Change short to u16 as well as modify tests to be si…
samuelorji Oct 7, 2023
ec5cf68
review feedback: add request timeout to execution profile to enable t…
samuelorji Oct 10, 2023
a23e6eb
review feedback: disable expensive batch write test for cassandra
samuelorji Oct 13, 2023
b35346b
review feedback
samuelorji Oct 13, 2023
6ecaa03
review feedback, use prepared statements
samuelorji Oct 17, 2023
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
4 changes: 2 additions & 2 deletions scylla-cql/src/frame/request/batch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ where
buf.put_u8(self.batch_type as u8);

// Serializing queries
types::write_short(self.statements.len().try_into()?, buf);
types::write_u16(self.statements.len().try_into()?, buf);

let counts_mismatch_err = |n_values: usize, n_statements: usize| {
ParseError::BadDataToSerialize(format!(
Expand Down Expand Up @@ -190,7 +190,7 @@ impl<'b> DeserializableRequest for Batch<'b, BatchStatement<'b>, Vec<SerializedV
fn deserialize(buf: &mut &[u8]) -> Result<Self, ParseError> {
let batch_type = buf.get_u8().try_into()?;

let statements_count: usize = types::read_short(buf)?.try_into()?;
let statements_count: usize = types::read_u16(buf)?.try_into()?;
let statements_with_values = (0..statements_count)
.map(|_| {
let batch_statement = BatchStatement::deserialize(buf)?;
Expand Down
26 changes: 25 additions & 1 deletion scylla-cql/src/frame/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use byteorder::{BigEndian, ReadBytesExt};
use bytes::{Buf, BufMut};
use num_enum::TryFromPrimitive;
use std::collections::HashMap;
use std::convert::TryFrom;
use std::convert::{Infallible, TryFrom};
use std::convert::TryInto;
use std::net::IpAddr;
use std::net::SocketAddr;
Expand Down Expand Up @@ -98,6 +98,12 @@ impl From<std::str::Utf8Error> for ParseError {
}
}

impl From<Infallible> for ParseError {
fn from(_: Infallible) -> Self {
ParseError::BadIncomingData("Unexpected Infallible Error".to_string())
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain what this trait implementation does?
AFAIU Infallible is for things that can never happen, so why do we want to convert it to a ParseError?

https://doc.rust-lang.org/std/convert/enum.Infallible.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's for the conversion of u16 to a usize. I wanted to do a simple as but didn't want to modify the existing code as much. I think converting from the previous i16 to a usize would have failed with the TryFromIntError error, but with u16 -> usize, it really is in fact infallible

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah okay, I understand now.
So we have a few pieces of code like this one:

let a: i16 = 123;
let b: usize = a.try_into()?;

And after changing the i16 to u16 they no longer compile because try_into() has an Infallible error type.
We can implement a conversion from Infallible to ParseError to make it compile, but it's a bit hacky.

I think it would be better to replace the try_into()?with into(), like this:

let a: u16 = 123;
let b: usize = a.into();

There is a an implentation of From<u16> for usize, so we can just use into() here.


impl From<std::array::TryFromSliceError> for ParseError {
fn from(_err: std::array::TryFromSliceError) -> Self {
ParseError::BadIncomingData("array try from slice failed".to_string())
Expand Down Expand Up @@ -174,10 +180,19 @@ pub fn read_short(buf: &mut &[u8]) -> Result<i16, ParseError> {
Ok(v)
}

pub fn read_u16(buf: &mut &[u8]) -> Result<u16, ParseError> {
let v = buf.read_u16::<BigEndian>()?;
Ok(v)
}

pub fn write_short(v: i16, buf: &mut impl BufMut) {
buf.put_i16(v);
}

pub fn write_u16(v: u16, buf: &mut impl BufMut) {
buf.put_u16(v);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should change write_short to accept u16.
AFAIU short refers to the CQL short type, which is a u16.
But we can do that in a separate PR as it will be a large change. Let's keep this one focused on the problem with Batches.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree about changing write_short to accept u16. There is only a small number of uses of this function in the code (I counted only six) so it should be easy to fix it right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The switch wasn't so simple, i had to change the type in some other structs that were linked in a way I don't understand yet. example. But as long as the tests pass, it should be fine.


pub(crate) fn read_short_length(buf: &mut &[u8]) -> Result<usize, ParseError> {
let v = read_short(buf)?;
let v: usize = v.try_into()?;
Expand All @@ -200,6 +215,15 @@ fn type_short() {
}
}

#[test]
fn type_u16() {
let vals = [0, 1, u16::MAX];
for val in vals.iter() {
let mut buf = Vec::new();
write_u16(*val, &mut buf);
assert_eq!(read_u16(&mut &buf[..]).unwrap(), *val);
}
}
// https://github.com/apache/cassandra/blob/trunk/doc/native_protocol_v4.spec#L208
pub fn read_bytes_opt<'a>(buf: &mut &'a [u8]) -> Result<Option<&'a [u8]>, ParseError> {
let len = read_int(buf)?;
Expand Down