-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
test/cqlpy: add regression test for tombstone_gc in "desc table" #22354
base: master
Are you sure you want to change the base?
Conversation
The small cqlpy test in this patch is a regression test for issue scylladb#14390, which claimed that the Scylla-only "tombstone_gc" option is missing from the output of "describe table". This test shows that this report is *not* true, at least not when the "server-side describe" is used. "test/cqlpy/run --release ..." shows that this test passes on master and also for Scylla versions all the way back to Scylla 5.2 (Scylla 5.1 did not support server-side describe, so the test fails for that reason). This suggests that the report in issue scylladb#14390 was for old-style client-side (cqlsh) describe, which we no longer support, so this issue can be closed. Fixes scylladb#14390. Signed-off-by: Nadav Har'El <nyh@scylladb.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
desc = cql.execute(f"DESCRIBE TABLE {table} WITH INTERNALS").one() | ||
# ignore spaces in comparison, as different versions of Scylla | ||
# add spaces in different places | ||
assert with_clause.replace(' ','') in desc.create_statement.replace(' ','') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could potentially be problematic if a new option whose suffix is tombstone_gc
were introduced, but for it to be an issue, it would also have to use the same parameters, so all in all it's unlikely to ever happen. Looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, considering that I'm looking for a very long string (tombstone_gc = {'mode': 'timeout', 'propagation_delay_in_seconds': '73'}
) there is no way it will match some other part of the "desc table" because of some unlucky coincidence. This is why I wanted to write this test as short as possible (just 5 lines) and not start to develop sophisticated text-processing machinary around it.
🟢 CI State: SUCCESS✅ - Build Build Details:
|
The small cqlpy test in this patch is a regression test for issue #14390, which claimed that the Scylla-only "tombstone_gc" option is missing from the output of "describe table".
This test shows that this report is not true, at least not when the "server-side describe" is used. "test/cqlpy/run --release ..." shows that this test passes on master and also for Scylla versions all the way back to Scylla 5.2 (Scylla 5.1 did not support server-side describe, so the test fails for that reason).
This suggests that the report in issue #14390 was for old-style client-side (cqlsh) describe, which we no longer support, so this issue can be closed.
Fixes #14390.