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

Some tests in test/cql-pytest/ fail on Cassandra #16027

Closed
nyh opened this issue Nov 12, 2023 · 6 comments · Fixed by #16033
Closed

Some tests in test/cql-pytest/ fail on Cassandra #16027

nyh opened this issue Nov 12, 2023 · 6 comments · Fixed by #16033
Assignees
Labels
area/cql area/test Issues related to the testing system code and environment

Comments

@nyh
Copy link
Contributor

nyh commented Nov 12, 2023

In issue #15969 I noticed that some tests in test/cql-pytest/test_permissions.py fail on Cassandra 4.1.
It appears that it's been a while since we tried to run these tests on Cassandra, and there are additional test files with tests that fail on Cassandra:

  • test_compaction_strategy_validation.py
  • test_describe.py
  • test_guardrail_replication_strategy.py
  • test_keyspace.py
  • test_materialized_view.py
  • test_secondary_index.py
  • cassandra_tests/validation/operations/compact_storage_test.py
  • cassandra_tests/validation/operations/select_single_column_relation_test.py

There isn't a lot to fix - most of these have just one failing test and most of the fixes are probably trivial. But we should do it.

@nyh nyh added area/test Issues related to the testing system code and environment area/cql labels Nov 12, 2023
@nyh nyh self-assigned this Nov 12, 2023
@nyh
Copy link
Contributor Author

nyh commented Nov 12, 2023

@Deexie I'm having some trouble with test_compaction_strategy_validation.py which you wrote and may need to create a new issue specifically for it. After I fix the differing error messages in Scylla and Cassandra, I find real differences between Scylla's and Cassandra's error cases which we should consider what, if at all, we want to do about.

In particular I noticed (this is probably an incomplete list):

  1. Trying to set tombstone_threshold to 5.5 is an error in ScyllaDB ("must be between 0.0 and 1.0") but allowed in Cassandra.
  2. Trying to set bucket_low to 0.0 is an error in ScyllaDB, giving the wrong-looking error message "must be between 0.0 and 1.0" (so 0.0 should have been fine?!) but allowed in Cassandra.
  3. Trying to set timestamp_resolution to SECONDS is an error in ScyllaDB ("invalid timestamp resolution SECONDS") but not an error in Cassandra. I don't think anybody wants to actually use "SECONDS", but it seems legal in Cassandra?

@Deexie
Copy link
Contributor

Deexie commented Nov 13, 2023

@Deexie I'm having some trouble with test_compaction_strategy_validation.py which you wrote and may need to create a new issue specifically for it. After I fix the differing error messages in Scylla and Cassandra, I find real differences between Scylla's and Cassandra's error cases which we should consider what, if at all, we want to do about.

In particular I noticed (this is probably an incomplete list):

1. Trying to set `tombstone_threshold` to 5.5 is an error in ScyllaDB ("must be between 0.0 and 1.0") but allowed in Cassandra.

0-1 restriction is mentioned in docs https://github.com/scylladb/scylladb/blob/master/docs/cql/compaction.rst.

2. Trying to set `bucket_low` to 0.0 is an error in ScyllaDB, giving the **wrong**-looking error message "must be between 0.0 and 1.0" (so 0.0 should have been fine?!) but allowed in Cassandra.

bucket_low equal to 0 degenerates the strategy. I tended to use between to indicate open interval, but I agree it's confusing.

3. Trying to set `timestamp_resolution` to `SECONDS` is an error in ScyllaDB ("invalid timestamp resolution SECONDS") but not an error in Cassandra. I don't think anybody wants to actually use "SECONDS", but it seems legal in Cassandra?

Here, an exception was thrown even before the validation. BTW. the option isn't mentioned in docs (neither oss nor enterprise).

@nyh
Copy link
Contributor Author

nyh commented Nov 14, 2023

1. Trying to set `tombstone_threshold` to 5.5 is an error in ScyllaDB ("must be between 0.0 and 1.0") but allowed in Cassandra.

0-1 restriction is mentioned in docs https://github.com/scylladb/scylladb/blob/master/docs/cql/compaction.rst.

It is mentioned in our documentation, but not in Cassandra's: https://cassandra.apache.org/doc/4.1/cassandra/operating/compaction/index.html

I agree that this limitation makes sense, but for some reason in this Cassandra unit test, they checked that 5.5 works, deliberately. I don't know why they wanted it to work. Perhaps we should look at their code to see what 5.5 does - I'm guessing it does the same as 1.0 and it's not really useful, but it's worth checking.

2. Trying to set `bucket_low` to 0.0 is an error in ScyllaDB, giving the **wrong**-looking error message "must be between 0.0 and 1.0" (so 0.0 should have been fine?!) but allowed in Cassandra.

bucket_low equal to 0 degenerates the strategy. I tended to use between to indicate open interval, but I agree it's confusing.

But again, bucket_low=0 is allowed in Cassandra and their test verified that it works. I don't know if it's useful, though. What does bucket_low mean - that every compaction is a major compaction? If this is the case, maybe somebody thought it was useful? Cassandra's documentation https://cassandra.apache.org/doc/stable/cassandra/operating/compaction/stcs.html doesn't say anything about this case.

In any case, if we decide that bucket_low=0 should be outlawed (@raphaelsc what do you think?) the error message should at least to use different wording that don't imply that maybe 0 is fine.

3. Trying to set `timestamp_resolution` to `SECONDS` is an error in ScyllaDB ("invalid timestamp resolution SECONDS") but not an error in Cassandra. I don't think anybody wants to actually use "SECONDS", but it seems legal in Cassandra?

Here, an exception was thrown even before the validation. BTW. the option isn't mentioned in docs (neither oss nor enterprise).

Right - we added the timestamp_resolution feature in 0c72781 but allowed only MILLISECOND (which was apparently used by KairosDB) , instead of allowing all the Java units. But the problem in this test (which you added in commit 14598fd) is that it assumes that "SECONDS" shouldn't work? Why? In Java it does. Arguably it should in Scylla too even if we never implemented. If you want to check an obvious error in units, use "DOGS", not "SECONDS" :-)

@nyh
Copy link
Contributor Author

nyh commented Nov 14, 2023

Opened issue #16057 about the missing support for timestamp_resolution = SECONDS. It's far from being important, but let's just remember we have this issue.

@mykaul
Copy link
Contributor

mykaul commented Jan 14, 2024

@nyh - are you going to backport #16033 to earlier releases?

@nyh
Copy link
Contributor Author

nyh commented Jan 14, 2024

@nyh - are you going to backport #16033 to earlier releases?

I don't think there's any reason to. The ability to run tests against Cassandra is useful for debugging the tests themselves - to check that the tests are verifying some really expecting behavior, rather than enshrining some misunderstood behavior of the code. We usually only run tests against Cassandra while developing new tests, or new features - on master. I never cared about doing this in old branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cql area/test Issues related to the testing system code and environment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants