Skip to content

SQL: relax version lock between server and clients#56148

Merged
bpintea merged 3 commits intoelastic:masterfrom
bpintea:enh/lift_version_lock_step
May 5, 2020
Merged

SQL: relax version lock between server and clients#56148
bpintea merged 3 commits intoelastic:masterfrom
bpintea:enh/lift_version_lock_step

Conversation

@bpintea
Copy link
Copy Markdown
Contributor

@bpintea bpintea commented May 4, 2020

This PR is to allow older-than-server clients to connect, if these are past or on a
certain min release, 7.7.0.
Note: the check is currently lower bounded only and subject to later adjustment to major minus one limitation.

Allow older-than-server clients to connect, if these are past or on a
certain min release.
@bpintea bpintea marked this pull request as ready for review May 4, 2020 19:49
@elasticmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-ql (:Query Languages/SQL)

@elasticmachine elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label May 4, 2020
@bpintea bpintea requested review from astefan, costin and matriv May 4, 2020 19:50
Copy link
Copy Markdown
Member

@costin costin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Copy Markdown
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Left a minor comment.

do {
prepareResponse(version);

String url = JdbcConfiguration.URL_PREFIX + webServerAddress();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those can be outside of the loop no?
and probably the whole try - catch can "wrap" the loop?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right, thanks!

Copy link
Copy Markdown
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Left one question, though.

return validationException;
}

protected boolean isClientCompatible() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think this method better belongs to SqlVersion class where hasVersionCompatibility() is also defined?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh, I had a hesitation initially, but it does belong there more naturally. Thanks for raising it.

- refactor VersionParityTests#testNoExceptionThrownForCompatibleVersions
to take idempotent assignments and try-catch blocks out a loop.
- move method that establishes version compatibiltiy into SqlVersion.
@bpintea bpintea requested review from astefan and matriv May 5, 2020 08:15
Copy link
Copy Markdown
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bpintea
Copy link
Copy Markdown
Contributor Author

bpintea commented May 5, 2020

@elasticmachine run elasticsearch-ci/bwc

@bpintea
Copy link
Copy Markdown
Contributor Author

bpintea commented May 5, 2020

@elasticmachine run elasticsearch-ci/default-distro

@bpintea
Copy link
Copy Markdown
Contributor Author

bpintea commented May 5, 2020

@elasticmachine update branch

@elasticmachine
Copy link
Copy Markdown
Collaborator

user doesn't have permission to update head repository

@bpintea bpintea merged commit 108f907 into elastic:master May 5, 2020
@bpintea bpintea deleted the enh/lift_version_lock_step branch May 5, 2020 15:15
bpintea added a commit to bpintea/elasticsearch that referenced this pull request May 5, 2020
* Relax version lock between ES/SQL and clients

Allow older-than-server clients to connect, if these are past or on a
certain min release.

(cherry picked from commit 108f907)
bpintea added a commit that referenced this pull request May 5, 2020
* Relax version lock between ES/SQL and clients

Allow older-than-server clients to connect, if these are past or on a
certain min release.

(cherry picked from commit 108f907)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants