- Notifications
You must be signed in to change notification settings - Fork 25.6k
Add deprecation check for Java version #64996
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
Conversation
Adds a deprecation check for Java version, as Java 11 will be required in the next version of Elasticsearch.
| Pinging @elastic/es-core-infra (:Core/Infra/Core) |
rjernst left a comment
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. One minor question: it looks like this will only exist in the Kibana migration UI, not in the deprecation log. Should we also have it in the latter?
| @rjernst this was already being logged in the |
Some tests check the length of the `node_settings` array that comes back from the `/_migration/deprecations` API. However, the length can now vary depending on the JDK being used by ES, and that can't be represented in the REST tests at the moment.
x-pack/plugin/src/test/resources/rest-api-spec/test/deprecation/10_basic.yml Show resolved Hide resolved
rjernst left a comment
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.
One more minor comment, still looks good.
| return new DeprecationIssue(DeprecationIssue.Level.CRITICAL, | ||
| "Java 11 is required", | ||
| "https://www.elastic.co/guide/en/elasticsearch/reference/master/breaking-changes-8.0.html#_java_11_is_required", | ||
| "Java 11 will be required for future versions of Elasticsearch, this node is running 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 wonder if we should first suggest using the bundled jdk?
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 idea. I've added a note here, and in a couple other places too. What do you think?
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 new wording is better, but it may be confusing for some users since they may have the bundled jdk, but have JAVA_HOME set by their system, so what they need to do is unset JAVA_HOME.
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 expanded the message further.
x-pack/plugin/src/test/resources/rest-api-spec/test/deprecation/10_basic.yml Show resolved Hide resolved
rjernst left a comment
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
| Thanks to @gwbrown for the initial implementation. |
Supersedes PR #41092.
Adds a deprecation check for Java version, as Java 11 will be required
in the next version of Elasticsearch.
Relates to #40756 and #40754.