search

scylladb / scylla-cqlsh

A fork of the cqlsh code
Apache License 2.0
16 stars 32 forks source link

handle server side DESCRIBE #17

Closed fruch closed 6 months ago

fruch commented 1 year ago

It was implemented on Scylla in:

https://github.com/scylladb/scylladb/pull/11106

scylla-cqlsh has the code to support it, but for now it's disabled for scylla, we need to find a way to identify the server side is supported

few options:

raphaelsc commented 1 year ago

Bumped into this today.

fruch commented 1 year ago

Bumped into this today.

What do you mean bumped, it was causing an issue ? or the describe didn't work as you expected ?

fruch commented 1 year ago

@roydahan this is not exactly specific to our group, but I think whom could take it, would learn lots in the process.

fruch commented 1 year ago

@avelanarius FYI this task is something I'm not getting to, and it sound like a something the driver team might suited to handle.

raphaelsc commented 1 year ago

Bumped into this today.

What do you mean bumped, it was causing an issue ? or the describe didn't work as you expected ?

By bumped, I meant that cqlsh doesn't use server-side describe by default, meaning that I had to user other means to access it. There are new attributes being introduced to Scylla that cannot be seen today with cqlsh.

avelanarius commented 1 year ago

few options:

  • based on scylla version
  • trying it on server side and fallback to client side
  • scylla to introduce a feature flags on a virtual table ?

I think "trying it on server side and fallback to client side" makes the most sense. I'm currently working on this issue (now only fixing tests which have hardcoded expected responses) and those are the reasons I listed:

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.
avelanarius commented 1 year ago

Refs https://github.com/scylladb/scylladb/issues/14895 - maybe I'll adjust the cqlsh tests to be more lenient towards missing whitespace, but it should be fixed in Scylla nevertheless.

mykaul commented 11 months ago

We should prioritize this (for https://github.com/scylladb/scylladb/issues/16482 , if we choose this path)

roydahan commented 11 months ago

@avelanarius I moved it back to you since I saw you wrote you were working on this issue. I'm not sure if you meant that you finished and bump into tests failures or started from the tests and stopped there. In any case, https://github.com/scylladb/scylladb/issues/14895 is fixed now.

Let's get done with this one as well.

mykaul commented 4 months ago

@roydahan - this went in without tests. In general, I'm not sure where's the CI for cqlsh and where are its own tests.

fruch commented 4 months ago

@roydahan - this went in without tests. In general, I'm not sure where's the CI for cqlsh and where are its own tests.

As part of the work in: https://github.com/scylladb/scylla-cqlsh/pull/88

@sylwiaszunejko tested it with dtest, there is a whole integration suite there for cqlsh, also covering describe.

Also the unit tests and use integration tests in cqlsh repo are covering, and were refactored during the work ion that PR.

mykaul commented 4 months ago

@roydahan - this went in without tests. In general, I'm not sure where's the CI for cqlsh and where are its own tests.

As part of the work in: #88

@sylwiaszunejko tested it with dtest, there is a whole integration suite there for cqlsh, also covering describe.

Also the unit tests and use integration tests in cqlsh repo are covering, and were refactored during the work ion that PR.

Great - thanks. I could not find the cqlsh tests.

Lorak-mmk commented 4 months ago

@roydahan - this went in without tests. In general, I'm not sure where's the CI for cqlsh and where are its own tests.

As part of the work in: #88

@sylwiaszunejko tested it with dtest, there is a whole integration suite there for cqlsh, also covering describe.

Also the unit tests and use integration tests in cqlsh repo are covering, and were refactored during the work ion that PR.

If dtests are the only tests for clqsh, then perhaps cqlsh CI should run them automatically?