SQL CLI requires strict version matching

[kcm]

elasticsearch/x-pack/plugin/sql/sql-cli/src/main/java/org/elasticsearch/xpack/sql/cli/Cli.java

Line 151 in 20e1b5e

". This version of CLI only works with Elasticsearch version " + Version.CURRENT.toString());

Perhaps this could check and indicate against minimumCompatibilityVersion() instead? It might misindicate if the SQL feature isn't properly represented in the logic, but it would be a step better than checking strict version equality.

cc @kurtado

[elasticmachine]

Pinging @elastic/es-search

[astefan]

@kcm I don't think this is a bug, as this was the initial intention with this functionality.
Any particular reason why you suggest a minimum compatibility version to be used instead?

[eskibars]

Agreed that I don't think it's a bug, but it's not particularly user friendly to require strict version matching. I think it's reasonable that a typical user will do things like rolling restart upgrades while leaving older cli utilities around (and expecting them to work).

[costin]

I agree with @eskibars that, at least for CLI, we should be more lenient and not force an upgrade every time the server upgrades as well however how and when we'll get around to do this, remains to be investigating.

Long story - off the top of my head:
For ODBC and JDBC leniency might be nice however it's just safer (especially for diagnostic) purposes to require an upgrade. OTOH some clients might not have the ability to upgrade as easily since the client lifecycle is quite different than that of the server (50 drivers spread across the network/apps vs a server in a centralized place).

A long with the CLI this leads to a versioning strategy similar to that of the server whoever separate from it since it affects only the SQL clients. So the same current/minimum/maximumCompatibilityVersion strategy might work however this needs more thinking before getting around implementing which is a risk for GA.

[nik9000]

With other clients we support forwards compatibility - an older client can talk to a server that is one major version newer. We should probably stick with that everywhere. The advantage of this is that it "fits in" perfectly with how we do versioning on the HTTP API, so long as clients are willing to ignore fields that they don't know.

I suspect we implemented strict version checking in the CLI because we figured we'd break the HTTP API because SQL was so new. If we're sure we're not going to then we can remove that check. Until we're sure, we might want to keep it. Maybe add some note about "because SQL is still beta" or something.

[kcm]

@nik9000 that's the gist of my intention, related to how we treat compatibility everywhere. I get the strict checking due to the SQL feature gray area, but then I kind of think it's a bug to wing it in that case. :)

[costin]

@kcm A bug implies incorrect behavior which is not the case; the intent is for all SQL consumers/drivers/clients to have the same version as the server.
The other clients in ES are consumers of a public REST API.
The odbc/jdbc are not; they are drivers that happen to use HTTP and JSON for their own internal protocol. The only guarantee that the drivers make is that they adhere to their respective API implementations.
Most (if not all) ODBC/JDBC drivers that I can think of require the same major version between the driver and the server.
In other words, as it stands, the driver and the server are bound by the serialization protocol.
The CLI is more like a driver as it uses protocol internally for formatting purposes. And such, it will be bound to the server version itself.
Hence why the forward compatibility promise is a big one as it adds a lot of burden and restrictions on the internal SQL protocol.

For example, adding INTERVALs meant adding a new SQL data type that the consumers had to understand. Allowing an old CLI to work with it would kinda work but it would display the data incorrectly (serialized format vs consumable format). Same will happen for geo types when they'll be introduced.

[j-bennet]

I downloaded a snapshot version, and the CLI is not woking, because Elasticsearch version does not match what it expects. Server version is 7.11.0-SNAPSHOT, so the CLI complains:

--- bin/elasticsearch-7.11.0-SNAPSHOT » bin/elasticsearch-sql-cli -v
WARNING: sun.reflect.Reflection.getCallerClass is not supported. This will impact performance.
ERROR: Cannot communicate with the server http://localhost:9200/. This version of CLI only works with Elasticsearch version 7.11.0

[bpintea]

Cannot communicate with the server http://localhost:9200/

@j-bennet Did u disable the authentication in ES? Since the URI seems otherwise to miss credentials.

[j-bennet]

@bpintea I did not provide an URI at all, this is the default.
If I provide an URI (with user:password in it), I'm able to log in. So I guess it's user error. It was a very unhelpful error message though:

Cannot communicate with the server http://localhost:9200/. This version of CLI only works with Elasticsearch version 7.11.0

I should have gotten a message about incorrect auth credentials.

[bpintea]

It was a very unhelpful error message though

That's true, we could probably do better here.

[elasticsearchmachine]

Pinging @elastic/es-analytical-engine (Team:Analytics)