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

test/cqlpy: add regression test for tombstone_gc in "desc table" #22354

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nyh
Copy link
Contributor

@nyh nyh commented Jan 16, 2025

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.

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>
Copy link
Contributor

@dawmd dawmd left a 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(' ','')
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@scylladb-promoter
Copy link
Contributor

🟢 CI State: SUCCESS

✅ - Build
✅ - Unit Tests

Build Details:

  • Duration: 5 hr 11 min
  • Builder: spider4.cloudius-systems.com

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/schema changes backport/none Backport is not required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

describe table doesn't show tombstone_gc option
4 participants