-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Upgrade Elasticsearch client and drop support for unsupported versions #11263
Conversation
I've removed all exclusions for now to see what fails and will clean up exclusions as required. |
cc: @martint Somehow it's working without any exclusions applied. Can you share some context on why the exclusions were added and if they may still be needed? Also I saw that even 6.8 got EOLed last month, so maybe in a few months we'll want to drop 6.8.x support for good as well. I've kept it here for now. |
There was a conflict with dependencies brought in by other components. It's possible those are already updated and the conflict no longer exists. |
I'll check the dependency tree before and after the change. Thanks for sharing context @martint. Would we want to drop 6.x altogether since it went EOL Feb 2022 or keep 6.8 for some time and then drop sometime in future? |
We should poll users to see what version they are running before dropping support for any version. |
56b4895
to
7ded8f8
Compare
3e7a113
to
d2a23e4
Compare
d2a23e4
to
e4ca92c
Compare
e4ca92c
to
63e2233
Compare
which tests are we talking about here? is this for a new pr? |
The "tests" there refers to all tests in the |
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.
looks good % #11263 (comment) taken care of
63e2233
to
966e3ba
Compare
See https://trinodb.slack.com/archives/CFLB9AMBN/p1655477455137409, https://trinodb.slack.com/archives/CP1MUNEUX/p1655450446249759 |
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! may be add a one-liner in commit description on why we're bumping client version.
The newer version addresses some critical CVEs and fixes some bugs regarding retry of failed requests being routed to dead hosts. It also makes the client work with ES 6.7 which had a breaking change causing older clients to not work with it in some cases. Elasticsearch clients are forward compatible and only guaranteed to work with servers with matching major versions. i.e. 6.8.23 client would work with 6.8.x and newer servers only. This also changes the TestElasticsearch6ConnectorTest to use 6.6.0 version for tests since that's the oldest version against which the connector passes all tests. This also adds comments explaining why an exclusion exists and also removes those that are no longer required - reducing the chances that someone runs into runtime errors if they do something which we don't explicitly test. Two new explicit dependencies on org.elasticsearch:elasticsearch-core and org.elasticsearch:elasticsearch-x-content are added because some classes from org.elasticsearch.common package were moved into separate modules.
966e3ba
to
4335822
Compare
Amended just the commit message - PTAL @phd3. |
We should definitely add this in the release notes and @colebow and myself can also mention it in the Trino Community Broadcast |
Description
Elasticsearch clients are forward compatible and only guaranteed to work
with servers with matching major versions. i.e. 6.8.23 client would work
with 6.8.x and newer servers only. This also changes the
TestElasticsearch6ConnectorTest to use 6.6.0 version for tests since
that's the oldest version against which the connector passes all tests.
This also adds comments explaining why an exclusion exists and also
removes those that are no longer required - reducing the chances that
someone runs into runtime errors if they do something which we don't
explicitly test.
Documentation
(x) Sufficient documentation is included in this PR.
Release notes
(x) Release notes entries required with the following suggested text: