-
Notifications
You must be signed in to change notification settings - Fork 25.2k
SQL: transfer version compatibility decision to the server #53082
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
SQL: transfer version compatibility decision to the server #53082
Conversation
This commit adds a new request object field, "request_version", containing the version of the requesting client. This parameter is now accepted by the server and the request is validated against it. Currently server's and client's versions need to be equal in order for the request to be accepted.
- change 'clientVersion' to 'version' in function names and members; - add testing for the validation of client's version.
Add the new 'client_version' field into the tests. Also: - add the 'client_version' field into proto/SqlClearCursorRequest.java builder. - remove one unused member (ConnectionConfiguration.java:CLIENT_ID).
This commit updates the clients to only check server's version against a lower bound: the server needs to always be on a higher version than the client. The server now enforces the version backwards compatibility. Currently, the versions still need to be in lockstep. Laxing this would be part of future work. The tests are updated to reflect the policy change and to have the version always included in query requests.
- add the version field into the request object of more tests
Pinging @elastic/es-search (:Search/SQL) |
@elasticmachine update branch |
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.
Left a first set of comments.
@@ -33,4 +33,9 @@ public String toString() { | |||
public static boolean isDriver(Mode mode) { | |||
return mode == JDBC || mode == ODBC; | |||
} | |||
|
|||
// TODO: replace all "== Mode.CLI"? | |||
public static boolean isCli(Mode mode) { |
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.
I don't see the usefulness of this method tbh. Mode is an enum, and there is only one CLI type of mode. Why not keeping the == Mode.CLI
? Things are different for isDriver
where is a double check, but for CLI why?
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.
In many cases the code look like Mode.isDriver(mode) || mode == Mode.CLI
and Mode.isDriver(mode) || Mode.isCli(mode)
looked more consistent to me. I got to it from a first thought of adding a Mode.isStrictClient()
to check for all drivers+cli (since we treat those grouped in many cases). Irrespective of that, though, if Mode being an enum is to be considered, one would either stick to mode == Mode.JDBC || mode == Mode.jdbc || mode == Mode.CLI
or the two methods wrapping, IMO.
However, this isn't a strictly necessary refactoring, it is subjective and I'll happily change it back if you still think the old way is better (just 👍 this comment).
@@ -26,7 +26,7 @@ | |||
|
|||
public void testProperConnection() throws Exception { | |||
HttpClient httpClient = mock(HttpClient.class); | |||
when(httpClient.serverInfo()).thenReturn(new MainResponse(randomAlphaOfLength(5), org.elasticsearch.Version.CURRENT.toString(), | |||
when(httpClient.serverInfo()).thenReturn(new MainResponse(randomAlphaOfLength(5), Version.CURRENT.toString(), |
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.
Why was this changed?
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.
The class has been making use of the client.Version
class already. Using two Version versions is not strictly needed anymore. Before this PR client.Version.toString()
behaved differently than elasticsearch.Version.toString()
, but this has been changed (old behavior is still available in client.Version.versionString()
, but that's not used anywhere atm.)
Version.fromString(major + "." + minor + ".23").toString(), | ||
ClusterName.DEFAULT.value(), UUIDs.randomBase64UUID())); | ||
CliSession cliSession = new CliSession(httpClient); | ||
expectThrows(ClientException.class, cliSession::checkConnection); |
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.
I think we should check the error message itself, as well.
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.
Sure. Neither of the already defined tests check the message, but I'll look into adding it.
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.
added.
|
||
|
||
|
||
public static final Version V_7_7_0 = new Version(7, 7, 0); |
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.
Maybe call this version something different. For example, in AbstractFieldHitExtractor
we have SWITCHED_FROM_DOCVALUES_TO_SOURCE_EXTRACTION
. Does it make sense to have this one called ENFORCED_CLIENT_VERSION_PARAMETER
?
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.
It could make sense, yes, but I imagine that we'll need to log a new version here with each "breaking" change we add in the protocol. If we'd instrument somehow these changes, referring to the versions by their number might be easier. But we could also refer to the versions by a name that conveys the gist of the breaking change, true. Wdyt?
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.
While I would prefer a name, I think using the versions gives us excellent flexibility in the long term. I'd opt towards adding a comment plus an utility method (that uses the field internally) with a verbose name:
public boolean supportsVersioning(Version version) {
return version.compareTo(V_7_...) >= 0;
}
s/assertThrows/assertRequestBuilderThrows as per elastic#52582
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.
Nice stuff! Left some comments.
ContentType.APPLICATION_JSON), mode); | ||
|
||
String cursor = (String) adminResponse.remove("cursor"); | ||
assertNotNull(cursor); | ||
|
||
final String m = randomMode(); | ||
ResponseException e = expectThrows(ResponseException.class, () -> RestActions.runSql("full_access", | ||
new StringEntity("{\"cursor\":\"" + cursor + "\"" + mode(m) + "}", ContentType.APPLICATION_JSON), m)); | ||
new StringEntity("{\"cursor\":\"" + cursor + "\"" + mode(m) + version(mode) + "}", ContentType.APPLICATION_JSON), |
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.
should it be version(m)
here?
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.
nice catch, thanks. fixed.
@@ -42,16 +42,16 @@ public void testErrorMessageForTranslatingSQLCommandStatement() throws IOExcepti | |||
|
|||
public void testErrorMessageForInvalidParamDataType() throws IOException { | |||
expectBadRequest(() -> runTranslateSql( | |||
"{\"query\":\"SELECT null WHERE 0 = ? \", \"mode\": \"odbc\", \"params\":[{\"type\":\"invalid\", \"value\":\"irrelevant\"}]}" | |||
), | |||
"{\"query\":\"SELECT null WHERE 0 = ? \", \"mode\": \"odbc\" " + version("odbc") + |
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.
maybe mode("odbc")
as well (also below)?
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.
makes sense, fixed.
@@ -56,8 +58,9 @@ public void writeTo(StreamOutput out) throws IOException { | |||
super.writeTo(out); | |||
out.writeEnum(requestInfo.mode()); | |||
out.writeOptionalString(requestInfo.clientId()); | |||
out.writeOptionalString(requestInfo.version() == null ? null : requestInfo.version().toString()); |
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.
since you use writeOptionalString
no need for null check.
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.
the check is there for the .toString()
to be safe.
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.
ah yep, I see, thx!
SqlClearCursorRequest::fromXContent); | ||
assertNull(request.clientId()); | ||
assertNull(request.version()); |
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.
Why is it null here?
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.
b/c the parser for SqlClearCursorRequest
doesn't set these fields.
public static final int MINOR_MULTIPLIER = REVISION_MULTIPLIER * REVISION_MULTIPLIER; | ||
public static final int MAJOR_MULTIPLIER = REVISION_MULTIPLIER * MINOR_MULTIPLIER; | ||
|
||
|
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.
nit: please keep one or two empy lines.
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.
sure. (i'm trying not to commit whitespace editor "fixes" - as everybody, prolly - and this one slipped through.)
- add exception message testing; - remove superfluous empty lines; - fix redefinition of mode string;
@elasticmachine update branch |
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.
Left a first round of comments.
@@ -110,7 +111,7 @@ private void assertCount(RestClient client, int count) throws IOException { | |||
expected.put("rows", singletonList(singletonList(count))); | |||
|
|||
Request request = new Request("POST", SQL_QUERY_REST_ENDPOINT); | |||
request.setJsonEntity("{\"query\": \"SELECT COUNT(*) FROM test\"" + mode(mode) + "}"); | |||
request.setJsonEntity("{\"query\": \"SELECT COUNT(*) FROM test\"" + mode(mode) + version(mode) + "}"); |
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.
It's worth adding an utility (or two) to handle the query wrapping:
- one to add mode() and version at the end as parameters
- another to wrap the given string in a query with the
{\"query\"
, escaped quotes and everything else.
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.
That's true. And the fact that the queries are "manually" build can lead to more work whenever the fields list is updated. I've had a stab at mimicking SqlQueryRequestBuilder's interface at building requests and apply the change through the tests here, but the change is a bit larger, and it might be worth having a separate PR?
@@ -70,10 +71,10 @@ public void expectMatchesAdmin(String adminSql, String user, String userSql) thr | |||
public void expectScrollMatchesAdmin(String adminSql, String user, String userSql) throws Exception { | |||
String mode = randomMode(); | |||
Map<String, Object> adminResponse = runSql(null, | |||
new StringEntity("{\"query\": \"" + adminSql + "\", \"fetch_size\": 1" + mode(mode) + "}", | |||
new StringEntity("{\"query\": \"" + adminSql + "\", \"fetch_size\": 1" + mode(mode) + version(mode) + "}", |
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.
Same comment as above, it applies in other places but I won't repeat it.
if (randomBoolean()) { | ||
// set it explicitly or leave the default (null) as is | ||
requestContent = new StringBuilder(requestContent) | ||
.insert(requestContent.length() - 1, ",\"binary_format\":" + binaryCommunication).toString(); | ||
binaryCommunication = ((Mode.isDriver(m) || m == Mode.CLI) && binaryCommunication); | ||
binaryCommunication = ((Mode.isDriver(mode) || Mode.isCli(mode)) && binaryCommunication); |
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.
Mode.isCli
doesn't add much value and since Mode.isDriver() || Mode.isCli
is a common pattern, it's worth wrapping that into its own method: Mode.isInternalClient
or Mode.isDedicatedClient
or Mode.isSqlClient
to differentiate between our "own" clients and everything else.
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.
Agreed, fixed it.
Mode mode = Mode.PLAIN; | ||
if (clientType.equals(ClientType.JDBC.toString())) { | ||
mode = Mode.JDBC.toString(); | ||
mode = Mode.JDBC; | ||
} else if (clientType.startsWith(ClientType.ODBC.toString())) { | ||
mode = Mode.ODBC.toString(); | ||
mode = Mode.ODBC; | ||
} else if (clientType.equals(ClientType.CLI.toString())) { | ||
mode = Mode.CLI.toString(); | ||
mode = Mode.CLI; | ||
} | ||
|
||
runSql(mode, clientType, sql); | ||
runSql(mode.toString(), clientType, sql); |
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.
👍
@@ -57,6 +63,7 @@ | |||
static final ParseField FILTER = new ParseField("filter"); | |||
static final ParseField MODE = new ParseField("mode"); | |||
static final ParseField CLIENT_ID = new ParseField("client_id"); | |||
static final ParseField CLIENT_VERSION = new ParseField("client_version"); |
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.
I would just say version
since all parameters are client specific. client_id
has is as such since just id
would have been too cryptic.
There's no client_mode, or client_params so why client_verision.
In fact, if anything we could change client_id to drop the prefix by using a different word: something like fingerprint or info (too loose).
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.
Makes sense, fixed it.
@@ -180,6 +180,9 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws | |||
if (clientId() != null) { | |||
builder.field("client_id", clientId()); | |||
} | |||
if (version() != null) { | |||
builder.field("client_version", version().toString()); |
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.
Some constants for these strings would be nice.
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.
Very much agreed. I would address this in a follow-up PR, thought, since these hardcoded strings are used extensively in tests?
|
||
|
||
|
||
public static final Version V_7_7_0 = new Version(7, 7, 0); |
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.
While I would prefer a name, I think using the versions gives us excellent flexibility in the long term. I'd opt towards adding a comment plus an utility method (that uses the field internally) with a verbose name:
public boolean supportsVersioning(Version version) {
return version.compareTo(V_7_...) >= 0;
}
@@ -16,37 +16,20 @@ | |||
import java.util.jar.JarInputStream; | |||
import java.util.jar.Manifest; | |||
|
|||
public class Version { | |||
public class Version extends org.elasticsearch.xpack.sql.proto.Version { |
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.
Functionally, this class is not another Version
but a consumer of Version
.
It has only isServerCompatible
and jdbcMajor/Minor
(which were added to this class since it was the only one but could be moved somewhere else).
Try refactoring it to something else either a consumer:
ClientVersion(Version) { .. }
or if there's no state, utility methods - ClientVersion/VersionUtils.isServerCompatible(version)`
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.
Sure. I've turned that class into a ClientVersion
utility class.
// as well. | ||
public static boolean isServerCompatible(Version server) { | ||
// Starting with this version, the compatibility logic moved from the client to the server. Only relevant for 7.x releases. | ||
return server.compareTo(Version.V_7_7_0) >= 0 |
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.
The V_7_7_0 constant is leaking - encapsulate that into Version (which already has the constant) to something like Version.hasVersionCompatibility/isCompatible
or something along those lines so the lines becomes:
return server.isCompatible()
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.
took on your .hasVersionCompatibility()
suggestion.
@@ -158,7 +158,7 @@ private void assertBinaryRequestForDrivers(boolean isBinary, XContentType xConte | |||
null, | |||
randomBoolean(), | |||
randomAlphaOfLength(128), | |||
new RequestInfo(mode), | |||
new RequestInfo(mode, Version.CURRENT), |
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.
See my comment on RequestInfo
constructor.
- change client.Version to a utility client.ClientVersion class; - rename proto.Version to proto.SqlVersion; - remove double initialization of 'version' in RequestInfo; - make V_7_7_0 version private and expose methods to compare versions against that; - add a Mode.isDedicatedClient(), remove Mode.isCli(); - renamed "client_version" field to just "version".
…asticsearch into feat/add_version_request_field
- VersionTests also updated with new ClientVersion class.
@elasticmachine run elasticsearch-ci/2 |
The client now only makes sure that server's version is above a certain limit (and not that that version is lower than own).
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.
Left few more comments.
@@ -72,7 +72,7 @@ public SqlQueryRequest(String query, List<SqlTypedParamValue> params, QueryBuild | |||
|
|||
@Override | |||
public ActionRequestValidationException validate() { | |||
ActionRequestValidationException validationException = null; | |||
ActionRequestValidationException validationException = super.validate(); |
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.
Why wasn't this called here before? The code we had was a bug?
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.
The overridden function (in action/AbstractSqlQueryRequest.java
) wasn't implemented before (and now it checks client's version).
+ "\"query\":\"select\"," | ||
+ "\"params\":[" + params + "]," | ||
+ " \"time_zone\":\"UTC\"," | ||
+ "\"request_timeout\":\"5s\",\"page_timeout\":\"10s\"}", SqlQueryRequest::fromXContent); | ||
assertNull(request.clientId()); | ||
assertEquals(randomMode, request.mode()); | ||
if (clientVersion.isEmpty() == false) { |
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.
Why not using the same condition as above? Mode.isDedicatedClient(randomMode)
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.
good point.
@@ -60,7 +60,7 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli | |||
*/ | |||
String accept = null; | |||
|
|||
if ((Mode.isDriver(sqlRequest.requestInfo().mode()) || sqlRequest.requestInfo().mode() == Mode.CLI) | |||
if ((Mode.isDedicatedClient(sqlRequest.requestInfo().mode())) |
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.
Too many round brackets?
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.
thanks, fixed.
- remove extra brackets - replace condition from string checking to client type checking
@elasticmachine run elasticsearch-ci/2 |
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
public static Version fromString(String version) { | ||
return new Version(version, "Unknown", from(version)); | ||
} | ||
public class ClientVersion { |
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.
Worth adding a javadoc to clarify the difference between ClientVersion
, SqlVersion
and Version
.
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.
+1
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.
I've added a short description for SqlVersion
and ClientVersion
. Hope it's enough, but could elaborate if felt as necessary.
Add short description of the SqlVersion and ClientVersion classes.
fix ref to unavailable org.elasticsearch.Version class
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
public void testConnection() throws Exception { | ||
HttpClient httpClient = mock(HttpClient.class); | ||
when(httpClient.serverInfo()).thenThrow(new SQLException("Cannot connect")); | ||
CliSession cliSession = new CliSession(httpClient); | ||
expectThrows(ClientException.class, cliSession::checkConnection); | ||
expectThrows(ClientException.class, cliSession::checkConnection, "java.sql.SQLException: Cannot connect"); |
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.
That's a good way of wrapping two checks in the same one, but throughout the code in SQL (where there are many such checks) we do them separately: expectThrows
and then another assertEquals
on the exception message.
I'm not saying you should change it, but the way it is now, it is inconsistent throughout sql code.
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.
adjusted to keep consistency.
/** | ||
* Clients-specific version utility class. | ||
* <p> | ||
* The class provides the SQL clients the version identifying the release their are part of. The version is read from the encompassing |
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.
they're
or they are
in the sentence identifying the release their are part of
.
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.
thanks, fixed.
@@ -33,4 +33,8 @@ public String toString() { | |||
public static boolean isDriver(Mode mode) { | |||
return mode == JDBC || mode == ODBC; | |||
} | |||
|
|||
public static boolean isDedicatedClient(Mode mode) { | |||
return mode == JDBC || mode == ODBC || mode == CLI; |
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.
return isDriver(mode) || mode == CLI
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.
Left some minor comments still, but LGTM.
- fix spelling - remove two-liner function
…53302) This commit adds a new request object field, "version", containing the version of the requesting client. This parameter is now accepted - and for certain clients required - by the server and the request is validated against it. Currently server's and client's versions still need to be equal in order for the request to be accepted. Relaxing this check is going to be part of future work. On the clients' side, the only check remaining is to ensure that the peer server is supporting version backwards compatibility (i.e. is on, or newer than a certain release). (cherry picked from commit a8f413a)
This param had been removed already along the elastic#53082 work.
* Document VarcharLimit and EarlyExecution params Add the documentation for the newly added VarcharLimit and EarlyExecution DSN attributes. * Remove obsolete VersionChecking param This param had been removed already along the #53082 work. * Update docs/reference/sql/endpoints/odbc/configuration.asciidoc fix typo Co-Authored-By: Stuart Cam <[email protected]> * Update docs/reference/sql/endpoints/odbc/configuration.asciidoc fix typo Co-Authored-By: Stuart Cam <[email protected]>
* Document VarcharLimit and EarlyExecution params Add the documentation for the newly added VarcharLimit and EarlyExecution DSN attributes. * Remove obsolete VersionChecking param This param had been removed already along the #53082 work. * Update docs/reference/sql/endpoints/odbc/configuration.asciidoc fix typo Co-Authored-By: Stuart Cam <[email protected]> * Update docs/reference/sql/endpoints/odbc/configuration.asciidoc fix typo Co-Authored-By: Stuart Cam <[email protected]> (cherry picked from commit f387616)
This PR transfer the decision to inter-communicate from the clients to the server.
The server will now accept a new request field that conveys the version of the client that originated the request.
For those clients that should have backwards compatibility support (cli, drivers), the server requires that this field be present in query (first/non-paginated) and translate requests.
In this first step, the service policy remains unchanged: the versions need to be in lockstep in order for the server to successfully answer it.
Similarly, the clients still do a server's version validation, but this is just for lower bounding: the server will need to be at least as recent as the client, or newer (plus a concrete lower bound, to prevent connections to older servers missing a version policy).
Closes #52766.