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

java 11 reflect warning fixes #284

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

java 11 reflect warning fixes #284

wants to merge 1 commit into from

Conversation

sjoerd-vogel
Copy link

@sjoerd-vogel sjoerd-vogel commented Nov 14, 2020

Problem

See #266, the current code throws warnings under jdk11

Solution

In jdk11 it is possible to avoid the illegal reflection by simply calling getters that now exist. I tried to stay java 8 backwards compatible by catching a possible NoSuchMethodError. The new code does need to be compiled using jdk11 however. Also its kinda hard to properly test this against jdk8.

Result

The warnings disappear in jdk11. Some additional (manual?) tests are required to check the jdk8 backwards compatibility.

@CLAassistant
Copy link

CLAassistant commented Nov 14, 2020

CLA assistant check
All committers have signed the CLA.

@ryanoneill
Copy link
Contributor

Hi @sjoerd-vogel! Thank you very much for submitting this pull request for util. Unfortunately, it's not going to be something that we can accept at the moment, because we are still using JDK/JVM 8 at Twitter. Hopefully that is resolved early next year. Until then, it means that we cannot call '.getVMManagement' directly.

Would it resolve the warnings you are seeing if the code was changed based on version to conditionally call '.getVMManagement' via reflection?

@sjoerd-vogel
Copy link
Author

sjoerd-vogel commented Nov 24, 2020

Hi @sjoerd-vogel! Thank you very much for submitting this pull request for util. Unfortunately, it's not going to be something that we can accept at the moment, because we are still using JDK/JVM 8 at Twitter. Hopefully that is resolved early next year. Until then, it means that we cannot call '.getVMManagement' directly.

Would it resolve the warnings you are seeing if the code was changed based on version to conditionally call '.getVMManagement' via reflection?

I dont think so. AFAIK you get this warning for any reflection on certain classes. Which is why the cast to the anonymous trait was problematic.

If there is anything I can do to speed up the migration of this module to jdk11 I would be happy to help. AFAIK you only need this module to compile against jdk11, I dont think you need to involve the other projects for this and you should still be backwards compatible for running this on older JVMs.

@ryanoneill
Copy link
Contributor

If there is anything I can do to speed up the migration of this module to jdk11 I would be happy to help. AFAIK you only need this module to compile against jdk11, I dont think you need to involve the other projects for this and you should still be backwards compatible for running this on older JVMs.

For our open source code such as this (Util, Finagle, Finatra, TwitterServer, Scrooge), it needs to work both internally and externally. Internally we have a monorepo with source dependencies, and so the cost must work with JDK 8 while the company is running JDK 8. So until we've migrated to JDK11 internally, which should happen sometime early next year, we don't have the ability to use any JDK code that does not compile with JDK 8.

Thank you for the offer to help with this. Unfortunately, my current understanding is that the vast majority of issues are in internal Twitter code.

@CEikermann
Copy link

@ryanoneill can you give us an update when this pull request can be merged?

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

Successfully merging this pull request may close these issues.

4 participants