search

twitter / util

Wonderful reusable code from Twitter
https://twitter.github.io/util
Apache License 2.0
2.69k stars 581 forks source link

java 11 reflect warning fixes #284

Open sjoerd-vogel opened 3 years ago

sjoerd-vogel commented 3 years ago

Problem

See https://github.com/twitter/util/issues/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 commented 3 years ago

CLA assistant check
All committers have signed the CLA.

ryanoneill commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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