cqlsh: try server-side DESCRIBE, then client-side

[sylwiaszunejko]

Since Scylla 5.2 (scylladb/scylladb@e6ffc22) a support for server-side DESCRIBE was added. However, cqlsh did not start to use this functionality, since it is only enabled if it detects CQL version at least 4. Scylla did not increase this version number as it doesn't support all of its features, so there is a need for a different detection mechanism for server-side DESCRIBE.

This PR changes the behavior in do_describe: cqlsh will first try to execute the server-side DESCRIBE. If this fails with SyntaxException, meaning that Scylla doesn't support that command, cqlsh falls back to the client-side DESCRIBE behavior. Also I fixed tests to be in line with recent changes to output sent by Scylla.

The other possible solutions were rejected:

• Based on Scylla version: would require ugly hard-coding of versions
• Modifying Scylla to provide some indication that this feature is enabled: Scylla 5.2 is already released without it, by implementing it in another way, we'll get it out sooner
• Do a trial "DESCRIBE" at the start of connection to detect if the server supports it: if cqlsh ever supported connecting to multiple nodes (right now it uses WhiteListRoundRobinPolicy) we would have to do the check on all of the nodes in case a rolling upgrade is currently occurring and some of the nodes don't support server-side DESCRIBE.

The change was tested manually on a couple of last Scylla OSS.

Fixes #17

[avelanarius]

One nit: "Fix tests" commit should be a part of the last commit - either @sylwiaszunejko should squash it or a scylla-cqlsh maintainer should merge it with squashing.

[piodul]

@fruch There are some dtests that use cqlsh to run DESCRIBE. Do we need to run them to see which dtests need fixing before this is merged?

[piodul]

cc: @Jadw1 - please review as well

[fruch]

@fruch There are some dtests that use cqlsh to run DESCRIBE. Do we need to run them to see which dtests need fixing before this is merged?

Yes we, otherwise we won't pass gating and won't be able to pass this change into scylla core

And we should be aware of those small differences in output, some of them might need fixes to the core, and might break tools/users

[Jadw1]

@fruch There are some dtests that use cqlsh to run DESCRIBE. Do we need to run them to see which dtests need fixing before this is merged?

Yes we, otherwise we won't pass gating and won't be able to pass this change into scylla core

And we should be aware of those small differences in output, some of them might need fixes to the core, and might break tools/users

Are those dtests using cqlsh or cql.execute()? Second option is fine since the statement is send directly to Scylla.
I'm not aware of all dtests, but I'm just pointing out the difference.

[fruch]

@fruch There are some dtests that use cqlsh to run DESCRIBE. Do we need to run them to see which dtests need fixing before this is merged?

Yes we, otherwise we won't pass gating and won't be able to pass this change into scylla core

And we should be aware of those small differences in output, some of them might need fixes to the core, and might break tools/users

Are those dtests using cqlsh or cql.execute()? Second option is fine since the statement is send directly to Scylla.
I'm not aware of all dtests, but I'm just pointing out the difference.

They are using cqlsh, no one wrote dtest for server side describe

[sylwiaszunejko]

@fruch How can I change cqlsh version in dtest to test my changes? And are these the tests you wrote about https://github.com/scylladb/scylla-dtest/blob/next/cqlsh_tests/cqlsh_tests.py?

[fruch]

@fruch How can I change cqlsh version in dtest to test my changes? And are these the tests you wrote about https://github.com/scylladb/scylla-dtest/blob/next/cqlsh_tests/cqlsh_tests.py?

You need to change the submodule in scylla core, and build the scylla unified package, then you can use that in dtest.

[sylwiaszunejko]

I have run the dtests and there are 9 tests failing with my changes:

FAILED cqlsh_tests/cqlsh_tests.py::TestCqlshWithSSL::test_int_values - AssertionError: Output 
FAILED cqlsh_tests/cqlsh_tests.py::TestCqlshWithSSL::test_describe - assert ['CREATE KEYS...d, col)', ...] == ['CRE...
FAILED cqlsh_tests/cqlsh_tests.py::TestCqlshWithSSL::test_describe_mv - AssertionError: assert ['CREATE MATE...me ASC)...
FAILED cqlsh_tests/cqlsh_tests.py::TestCqlsh::test_int_values - AssertionError: Output 
FAILED cqlsh_tests/cqlsh_tests.py::TestCqlsh::test_describe - assert ['CREATE KEYS...d, col)', ...] == ['CRE...
FAILED cqlsh_tests/cqlsh_tests.py::TestCqlsh::test_describe_mv - AssertionError: assert ['CREATE MATE...me ASC)...
FAILED cqlsh_tests/cqlsh_tests.py::TestCqlLogin::test_login_keeps_keyspace - AssertionError: assert ['ks1table'] == ['ks1ta...

I will now try to fix it.

[fruch]

I have run the dtests and there are 9 tests failing with my changes:

FAILED cqlsh_tests/cqlsh_tests.py::TestCqlshWithSSL::test_int_values - AssertionError: Output 
FAILED cqlsh_tests/cqlsh_tests.py::TestCqlshWithSSL::test_describe - assert ['CREATE KEYS...d, col)', ...] == ['CRE...
FAILED cqlsh_tests/cqlsh_tests.py::TestCqlshWithSSL::test_describe_mv - AssertionError: assert ['CREATE MATE...me ASC)...
FAILED cqlsh_tests/cqlsh_tests.py::TestCqlsh::test_int_values - AssertionError: Output 
FAILED cqlsh_tests/cqlsh_tests.py::TestCqlsh::test_describe - assert ['CREATE KEYS...d, col)', ...] == ['CRE...
FAILED cqlsh_tests/cqlsh_tests.py::TestCqlsh::test_describe_mv - AssertionError: assert ['CREATE MATE...me ASC)...
FAILED cqlsh_tests/cqlsh_tests.py::TestCqlLogin::test_login_keeps_keyspace - AssertionError: assert ['ks1table'] == ['ks1ta...

I will now try to fix it.

if you are fixing the dtest, I would suggest doing it in a way it would work for both (so we can merge before, and not force us to merge it at the same time)

[sylwiaszunejko]

The PR with changes to the dtests is now merged https://github.com/scylladb/scylla-dtest/pull/4319 and I did some refactoring of the tests here to make it work for different formatting versions.
@fruch @avelanarius @piodul

[fruch]

LGTM