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

fix(utils): generate time values in the correct range #389

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

cvybhu
Copy link
Contributor

@cvybhu cvybhu commented Jul 12, 2023

The time CQL type represents a duration of less than 24 hours, represented as int64_t nanoseconds:

https://github.com/apache/cassandra/blob/f5df4b219e063cb24b9cc0c22b6e614506b8d903/doc/native_protocol_v4.spec#L941

6.16 time

  An 8 byte two's complement long representing nanoseconds since midnight.
  Valid values are in the range 0 to 86399999999999

time is an int64 in range [0;86399999999999], values outside of this range are invalid.

RandTime() generated values using rnd.Int63(), but this generates values outside of the allowed range.

Let's fix it so that it generates values in the correct range.

The generation of time values was recently modified in #359, and time values have been enabled in tests, but this caused failures on Gemini runs: scylladb/scylladb#14645

Fixes: scylladb/scylladb#14645
Refs: scylladb/scylladb#14667

/cc @illia-li

According to the CQL binary protocol, time is an int64 in range [0;86399999999999]

https://github.com/apache/cassandra/blob/f5df4b219e063cb24b9cc0c22b6e614506b8d903/doc/native_protocol_v4.spec#L941
An 8 byte two's complement long representing nanoseconds since midnight.
Valid values are in the range 0 to 86399999999999

Values outside of this range are invalid.

RandTime() generated values using rnd.Int63(),
but this generates values outside of the allowed range.

Let's fix it so that it generates values in the correct range.

Fixes: scylladb/scylladb#14645
Refs: scylladb/scylladb#14667

Signed-off-by: Jan Ciolek <[email protected]>
@cvybhu cvybhu requested a review from dkropachev July 12, 2023 14:48
Copy link

@nyh nyh left a comment

Choose a reason for hiding this comment

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

Looks good.

By the way, as a Go newbie, I got thrown off by the function name Int63n that Go uses for the random number generator. It's so odd :-(

@dkropachev dkropachev merged commit 232dcbc into scylladb:master Jul 12, 2023
@yarongilor
Copy link

yarongilor commented Jul 13, 2023

@dkropachev , just a side comment related to scylladb/scylladb#14645 -

The error was not found by Gemini itself, Gemini just kept running. It happened when i manually connected cqlsh and run this query. So i wonder if Gemini actually has an ability of running a select-query for each one of the columns it generated.

@dkropachev
Copy link
Collaborator

@dkropachev , just a side comment related to scylladb/scylladb#14645 -

The error was not found by Gemini itself, Gemini just kept running. It happened when i manually connected cqlsh and run this query. So i wonder if Gemini actually has an ability of running a select-query for each one of the columns it generated.

Not for each one, but it run selects on values that where inserted before

@nyh
Copy link

nyh commented Jul 13, 2023

@yarongilor it's still not clear to me if only cqlsh fails when reading the "time" value in the unexpected range, or whether the Python driver and/or other drivers also fail in the same way. I would guess that many drivers would not bother to check the 64-bit value they read for validity (being 0<=t<24h), so it's possible that Gemini did read the random data it wrote with the Go driver (right?) - and thought everything was fine.

@yarongilor
Copy link

yarongilor commented Jul 13, 2023

@yarongilor it's still not clear to me if only cqlsh fails when reading the "time" value in the unexpected range, or whether the Python driver and/or other drivers also fail in the same way. I would guess that many drivers would not bother to check the 64-bit value they read for validity (being 0<=t<24h), so it's possible that Gemini did read the random data it wrote with the Go driver (right?) - and thought everything was fine.

The failure only happened via cqlsh.
@nyh , regarding go driver handling - this sounds likely to happen but i cannot confirm that.
BTW, do you think it could be useful to use cqlsh querying Gemini generated data? can it be interesting in order to expose similar scylla issues?

@nyh
Copy link

nyh commented Jul 13, 2023

@yarongilor it's still not clear to me if only cqlsh fails when reading the "time" value in the unexpected range, or whether the Python driver and/or other drivers also fail in the same way. I would guess that many drivers would not bother to check the 64-bit value they read for validity (being 0<=t<24h), so it's possible that Gemini did read the random data it wrote with the Go driver (right?) - and thought everything was fine.

The failure only happened via cqlsh. @nyh , regarding go driver handling - this sounds likely to happen but i cannot confirm that. BTW, do you think it could be useful to use cqlsh querying Gemini generated data? can it be interesting in order to expose similar scylla issues?

Personally, I don't think so. This will never end - next year somebody will complain that we didn't check some interaction of the Rust driver with Scylla so we should have checked it in Gemini. I think the place to test the Rust driver is in the Rust driver test suite, and the place to test cqlsh is in a cqlsh test suite (if there's even one). I don't think that Gemini should be responsible for that.

@cvybhu
Copy link
Contributor Author

cvybhu commented Jul 13, 2023

it's still not clear to me if only cqlsh fails when reading the "time" value in the unexpected range, or whether the Python driver and/or other drivers also fail in the same way

Python driver has the check here: https://github.com/scylladb/python-driver/blob/18ea6d47b2d9cf5bc5e7b7eed32d3bf647b71c97/cassandra/util.py#L1000

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