mail-api icon indicating copy to clipboard operation
mail-api copied to clipboard

CompactFormatter support for LogRecord::getLongThreadID #529

Open jmehrens opened this issue 3 years ago • 6 comments

https://github.com/eclipse-ee4j/mail/issues/529

This adds support for long thread ids when running JDK 16. I'm looking to backport this to 1.x. when this is approved.

I updated other methods to use method handles over reflection where the code is accessing JDK classes. The remaining methods that use reflection are not hotspots. I attempted to use method handles on the remaining methods that still use reflection and it seems that the access rules are a bit different with regards to private classes with public methods. In a follow on ticket I'll update the 2.0.1 version of the logging package to make direct methods calls since the min JDK version is 11. That will clean up most uses of reflection.

@lukasj Once this is approved I can squash merge this into master branch.

jmehrens avatar Mar 06 '21 18:03 jmehrens

well, we still have to support JDK 8 in 2.0.x. The path to go here can be multi-release jar. Basically what it needs is just a new folder under project's resources (ie java-mr/9, new compiler-plugin execution and updating jar's manifest to contain Multi-Release: true header (and updating exclusions in OSGi headers with updating parts which are repackaging affected project, if there are any), see ie this config for an example

lukasj avatar Mar 08 '21 16:03 lukasj

@lukasj

well, we still have to support JDK 8 in 2.0.x.

Makes sense. So we are build on JDK 11 and targeting JDK8. That reminds me we need to get that we are using release option and not the source/target options.

I've tested the current PR against JDK8, JDK11, and JDK16. I tested backporting of this code to 1.x under JDK7 which all tests passed. The way this works if the the API methods are available from the JDK they are used. Otherwise fallback and use API method appropriate for the release.

The path to go here can be multi-release jar. Basically what it needs is just a new folder under project's resources (ie java-mr/9, new compiler-plugin execution and updating jar's manifest to contain Multi-Release: true header (and updating exclusions in OSGi headers with updating parts which are repackaging affected project, if there are any), see ie this config for an example

This makes sense. It doesn't look like this PR will make 2.0.1 which is fine but I'll need to update the since javadoc tag and changes.txt before merge when next release is started. Not an issue I'll wait and watch the master branch.

The current logging package source assumes JDK7 source for both 1x and 2.0.X. So After this PR, I'll create a ticket updating source to use JDK8 methods for 2.0.x and create a ticket for multi-release.

jmehrens avatar Mar 08 '21 21:03 jmehrens

@lukasj I cherry-picked the changes to remove method handles from 2.0.2 version. I also cleaned up some code smells and added some references to Android. This will have to be squashed and merged to clean up the multi-commits.

jmehrens avatar Apr 08 '21 22:04 jmehrens

Do we want to still support "old" android versions in master/2.x? I'd prefer we do a step forward here and those who need it, can use 1.x. I'm just asking - what do you think?

lukasj avatar Apr 08 '21 22:04 lukasj

This is confusion on my part. What is the minimum Android API version for supported for master/2.x? For v1.x it is listed as 19 per https://eclipse-ee4j.github.io/mail/Android but I don't see where we declare min version for 2.x. If it is later than 26 I can pull back the method handles version.

jmehrens avatar Apr 09 '21 04:04 jmehrens

@jmehrens I think it is time to move on with jakarta, let's target API 26+ (or whatever min version mhandles require)

lukasj avatar Jul 27 '21 15:07 lukasj

This work will be continued in AngusMail.

jmehrens avatar Dec 22 '22 23:12 jmehrens