Skip to content

Conversation

@pugnascotia
Copy link
Contributor

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.

@pugnascotia pugnascotia added :Core/Infra/Core Core issues without another label v7.11.0 labels Nov 12, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Core)

@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Nov 12, 2020
Copy link
Member

@rjernst rjernst left a 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?

@pugnascotia
Copy link
Contributor Author

@rjernst this was already being logged in the Bootstrap class.

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.
@pugnascotia pugnascotia requested a review from rjernst November 16, 2020 12:10
Copy link
Member

@rjernst rjernst left a 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 "
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM

@pugnascotia pugnascotia merged commit 2d0b012 into elastic:7.x Nov 19, 2020
@pugnascotia pugnascotia deleted the deprecation-check-java branch November 19, 2020 09:23
@pugnascotia
Copy link
Contributor Author

Thanks to @gwbrown for the initial implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Core Core issues without another label >enhancement Team:Core/Infra Meta label for core/infra team v7.11.0

4 participants