Skip to content

Suppress illegal reflective access in shared cache #70344

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

Merged

Conversation

jasontedor
Copy link
Member

This commit temporarily supressess an illegal reflective access warning by opening the java.io module to all unnamed modules. This is needed when using the searchable snapshots shared cache due to some reflective access that occurs there. This is temporary while we explore alternatives.

Relates #70285

This commit temporarily supressess an illegal reflective access warning
by opening the java.io module to all unnamed modules. This is needed
when using the searchable snapshots shared cache due to some reflective
access that occurs there. This is temporary while we explore
alternatives.
@jasontedor jasontedor added >non-issue :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs :Core/Infra/Core Core issues without another label v8.0.0 v7.12.0 v7.13.0 labels Mar 12, 2021
@jasontedor jasontedor requested a review from rjernst March 12, 2021 02:23
@elasticmachine elasticmachine added Team:Core/Infra Meta label for core/infra team Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. labels Mar 12, 2021
@elasticmachine
Copy link
Collaborator

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

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

@jasontedor
Copy link
Member Author

This is to suppress these lines that get dumped to stderr:

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.elasticsearch.xpack.searchablesnapshots.preallocate.Preallocate$FileDescriptorFieldAction (file:/Users/jason/src/elastic/elasticsearch/build/distribution/local/elasticsearch-8.0.0-SNAPSHOT/modules/searchable-snapshots/preallocate-8.0.0-SNAPSHOT.jar) to field java.io.FileDescriptor.fd
WARNING: Please consider reporting this to the maintainers of org.elasticsearch.xpack.searchablesnapshots.preallocate.Preallocate$FileDescriptorFieldAction
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

It is a bad user experience to encounter this output.

Yes, this solution is a hammer to suppress them, but there aren't any obvious alternatives given that we are running without modules, and therefore the only way we can grant the necessary opens here are to all unnamed modules. If we were running with modules, we could open this only to the necessary module, preferably by placing the opens declaration in the JAR manifest itself.

@jasontedor
Copy link
Member Author

And we will encounter a lot of noise from this line:

WARNING: Please consider reporting this to the maintainers of org.elasticsearch.xpack.searchablesnapshots.preallocate.Preallocate$FileDescriptorFieldAction

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.

What do you think about putting it in SystemJvmOptions since it is an implementation detail of the server rather than something the user should even need to think about?

@jasontedor
Copy link
Member Author

@rjernst SystemJvmOptions doesn't allow us to switch on JVM version, which we need to be able to do here so that in 7.x on Java 8 we don't emit this JVM option. We can talk about adding that capability or not, so far we haven't needed it in SystemJvmOptions, but I don't think that should block this change. Additionally, we now tell users not to edit the root jvm.options.

@jasontedor jasontedor requested a review from rjernst March 12, 2021 10:56
@jasontedor
Copy link
Member Author

Actually @rjernst in #54853 I already used versioning logic in SystemJvmOptions 🤦‍♀️:

private static String maybeShowCodeDetailsInExceptionMessages() {
if (JavaVersion.majorVersion(JavaVersion.CURRENT) >= 14) {
return "-XX:+ShowCodeDetailsInExceptionMessages";
} else {
return "";
}
}

I've pushed dd271d1 and when backporting to 7.12 and 7.x will do the same here.

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

@rjernst rjernst merged commit b6eb6e3 into elastic:master Mar 15, 2021
@jasontedor jasontedor deleted the shared-cache-illegal-reflective-access branch March 15, 2021 23:23
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 :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >non-issue Team:Core/Infra Meta label for core/infra team Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v7.12.0 v7.13.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants