Skip to content
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

Enable unmap hack for java 9 #16986

Merged
merged 1 commit into from
Mar 7, 2016
Merged

Enable unmap hack for java 9 #16986

merged 1 commit into from
Mar 7, 2016

Conversation

rmuir
Copy link
Contributor

@rmuir rmuir commented Mar 7, 2016

This mechanism changed for java 9: requires a "fake" permission of accessClassInPackage.jdk.internal.ref

Currently unmapping will not work with java 9 unless we make changes:

  2> REPRODUCE WITH: gradle :core:test -Dtests.seed=D11864EB2E98C3EF -Dtests.class=org.elasticsearch.common.lucene.LuceneTests -Dtests.method="testMMapHackSupported" -Des.logger.level=WARN -Dtests.security.manager=true -Dtests.locale=jmc-TZ -Dtests.timezone=Antarctica/DumontDUrville
FAILURE 0.13s J0 | LuceneTests.testMMapHackSupported <<< FAILURES!
   > Throwable #1: java.lang.AssertionError: MMapDirectory does not support unmapping: Unmapping is not supported, because not all required permissions are given to the Lucene JAR file: java.security.AccessControlException: access denied ("java.lang.RuntimePermission" "accessClassInPackage.jdk.internal.ref") [Please grant at least the following permissions: RuntimePermission("accessClassInPackage.sun.misc"), RuntimePermission("accessClassInPackage.jdk.internal.ref"), and ReflectPermission("suppressAccessChecks")]

See https://issues.apache.org/jira/browse/LUCENE-6989 for more information.

I also had to upgrade my gradle to the latest to work at all with java 9, and fix a test so the source code compiles at all. I only ran this small unit test because hotspot on java 9 has been broken for a long time.

@rmuir rmuir added the >bug label Mar 7, 2016
@uschindler
Copy link
Contributor

Looks good.

@mikemccand
Copy link
Contributor

+1, thanks @rmuir. It's important we can run under Java 9 ea even if recent builds have been badly broken...

rmuir added a commit that referenced this pull request Mar 7, 2016
Enable unmap hack for java 9
@rmuir rmuir merged commit 2dd8ed9 into elastic:master Mar 7, 2016
@@ -109,7 +109,12 @@ private void createStaleReplicaScenario() throws Exception {
logger.info("--> check that old primary shard does not get promoted to primary again");
// kick reroute and wait for all shard states to be fetched
client(master).admin().cluster().prepareReroute().get();
assertBusy(() -> assertThat(internalCluster().getInstance(GatewayAllocator.class, master).getNumberOfInFlightFetch(), equalTo(0)));
Copy link
Contributor

Choose a reason for hiding this comment

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

was this change wanted, it looks unrelated to this issue?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as mentioned in the issue, it will not compile with java 9.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. That seems to be a bug in javac :-) Maybe we should open issue!

Copy link
Contributor

Choose a reason for hiding this comment

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

I would have tried:

assertBusy(() -> {
  assertThat(internalCluster().getInstance(GatewayAllocator.class, master).getNumberOfInFlightFetch(), equalTo(0));
});

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 gave it the obvious fix without any special sugar. and plenty of assertBusy's, actually most of them, are written this way.

again, as it was a compiler failure, the style is unimportant to me really. More important was to get mmap working :)

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

Successfully merging this pull request may close these issues.

None yet

4 participants