elonazoulay commented 3 years ago

Description

As part of effort to upgrade Pinot to use JAVA 11(#6689), this PR will:

Upgrade pinot to be compatible with jdk11+. This involves updating dependencies, tests and minor modifications to some classes: equality methods for Schema and ColumnMetadata.

JDK 8 build is still available with the option: -Djdk.version=8

mvn clean install -DskipTests -Pbin-dist -T 4  -Djdk.version=8

Upgrade Notes

Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)

[ X] Yes (Please label as backward-incompat, and complete the section below on Release Notes)

Does this PR fix a zero-downtime upgrade introduced earlier?

[ ] Yes (Please label this as backward-incompat, and complete the section below on Release Notes)

Does this PR otherwise need attention when creating release notes? Things to consider:

New configuration options
Deprecation of configurations
Signature changes to public methods/interfaces
New plugins added or old plugins removed
[ X] Yes (Please label this PR as release-notes and complete the section on Release Notes)
Release Notes

Upgrade to Java 11 and drop support for Java 8

If you have tagged this as either backward-incompat or release-notes, you MUST add text here that you would like to see appear in release notes of the next release.

If you have a series of commits adding or enabling a feature, then add this section only in final commit that marks the feature completed. Refer to earlier release notes to see examples of text

Documentation

If you have introduced a new feature or configuration, please add it to the documentation as well. See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document

xiangfu0 commented 3 years ago

delete this gc.log.*?

Jackie-Jiang commented 3 years ago

What are we trying to achieve here? Should we provide multiple docker files for different java versions?

elonazoulay commented 3 years ago

What are we trying to achieve here? Should we provide multiple docker files for different java versions?

It seems that java11+ should be used. Let me know if there is any usecase where java10 should be added back (I removed java 1.8 and 10 from the versions to be tested).

codecov-commenter commented 3 years ago

Codecov Report

Merging #6424 (6b3ed47) into master (a1c9b63) will increase coverage by 0.23%. The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #6424      +/-   ##
============================================
+ Coverage     73.38%   73.62%   +0.23%     
- Complexity       12       91      +79     
============================================
  Files          1453     1476      +23     
  Lines         72032    72680     +648     
  Branches      10427    10449      +22     
============================================
+ Hits          52863    53508     +645     
- Misses        15643    15712      +69     
+ Partials       3526     3460      -66

Flag	Coverage Δ
integration	`41.99% <0.00%> (-0.14%)`	:arrow_down:
unittests	`65.50% <100.00%> (+0.10%)`	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...org/apache/pinot/spi/plugin/PluginClassLoader.java	`25.53% <100.00%> (+1.00%)`	:arrow_up:
...ot/segment/local/customobject/MinMaxRangePair.java	`75.86% <0.00%> (-24.14%)`	:arrow_down:
...g/apache/pinot/segment/spi/memory/CleanerUtil.java	`33.87% <0.00%> (-23.28%)`	:arrow_down:
...n/src/main/java/org/apache/pinot/common/Utils.java	`40.42% <0.00%> (-19.15%)`	:arrow_down:
...e/pinot/core/transport/InstanceRequestHandler.java	`62.31% <0.00%> (-16.26%)`	:arrow_down:
.../apache/pinot/core/transport/DataTableHandler.java	`88.00% <0.00%> (-12.00%)`	:arrow_down:
...pache/pinot/core/query/utils/idset/EmptyIdSet.java	`25.00% <0.00%> (-10.72%)`	:arrow_down:
...re/operator/dociditerators/EmptyDocIdIterator.java	`75.00% <0.00%> (-8.34%)`	:arrow_down:
...inion/event/DefaultMinionEventObserverFactory.java	`66.66% <0.00%> (-8.34%)`	:arrow_down:
...impl/dictionary/DoubleOnHeapMutableDictionary.java	`46.98% <0.00%> (-6.03%)`	:arrow_down:
... and 348 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e91f426...6b3ed47. Read the comment docs.

xiangfu0 commented 3 years ago

What are we trying to achieve here? Should we provide multiple docker files for different java versions?

It seems that java11+ should be used. Let me know if there is any usecase where java10 should be added back (I removed java 1.8 and 10 from the versions to be tested).

I think it's ok to bypass JDK 9 and 10.

xiangfu0 commented 3 years ago

Can you also try to remove this --add-exports java.base/jdk.internal.ref=ALL-UNNAMED flag in file: pinot-tools/src/main/resources/appAssemblerScriptTemplate to see if it can pass the ci? With your change I think we can safely remove this tag.

elonazoulay commented 3 years ago

Can you also try to remove this --add-exports java.base/jdk.internal.ref=ALL-UNNAMED flag in file: pinot-tools/src/main/resources/appAssemblerScriptTemplate to see if it can pass the ci? With your change I think we can safely remove this tag.

Trying it now!

kishoreg commented 3 years ago

pardon my ignorance - why do we say that this change makes it backward-incompatible?

elonazoulay commented 3 years ago

pardon my ignorance - why do we say that this change makes it backward-incompatible?

By backward incompatible do you mean that an older pinot cluster cannot be upgraded? If so I would say this is not backward-incompatible: we were able to upgrade a cluster in place with the same configuration.

@xiangfu0 do you have some more context?

xiangfu0 commented 3 years ago

pardon my ignorance - why do we say that this change makes it backward-incompatible?

By backward incompatible do you mean that an older pinot cluster cannot be upgraded? If so I would say this is not backward-incompatible: we were able to upgrade a cluster in place with the same configuration.

@xiangfu0 do you have some more context?

I think the issue is that JDK11 built binary cannot be ran using JDK8, which requires all existing JDK8 users to upgrade their environment.

Here are the errors when you try to run JDK 11 built pinot with JDK 8 Runtime:

Error: A JNI error has occurred, please check your installation and try again
Exception in thread "main" java.lang.UnsupportedClassVersionError: org/apache/pinot/tools/Quickstart has been compiled by a more recent version of the Java Runtime (class file version 55.0), this version of the Java Runtime only recognizes class file versions up to 52.0
    at java.lang.ClassLoader.defineClass1(Native Method)
    at java.lang.ClassLoader.defineClass(ClassLoader.java:756)
    at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142)
    at java.net.URLClassLoader.defineClass(URLClassLoader.java:468)
    at java.net.URLClassLoader.access$100(URLClassLoader.java:74)
    at java.net.URLClassLoader$1.run(URLClassLoader.java:369)
    at java.net.URLClassLoader$1.run(URLClassLoader.java:363)
    at java.security.AccessController.doPrivileged(Native Method)
    at java.net.URLClassLoader.findClass(URLClassLoader.java:362)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:418)
    at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:352)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:351)
    at sun.launcher.LauncherHelper.checkAndLoadMain(LauncherHelper.java:495)

xiangfu0 commented 3 years ago

Another thing we can try is to add the jdk.version override. E.g.

mvn clean install -Pbin-dist  -DskipTests -Djdk.version=8

Then users may still be able to use JDK 8 to build and run Pinot.

However it may result we need to release two versions of Pinots based on JDK.

xiangfu0 commented 3 years ago

I tested on JDK 8 build, seems that it can work, we just need to change the plugin loader step to have similar behavior as the JDK9+.

Also not yet test the behavior for Hadoop and Spark. It may require putting all the jars into the classpath.

xiangfu0 commented 3 years ago

Updated the PR to make it compilable with JAVA 8. Make tests to use java 11 and quickstart can be built and ran by java 8/10

xiangfu0 commented 2 years ago

LGTM. My only concern is how do we ensure the tests can pass in java 8? We still want to keep java 8 supported

We want to keep java 8 support, so we keep the quickstart test for java 8, but no unit tests or integration tests.

Also, this setup blocks using the code feature of java 11.

Jackie-Jiang commented 2 years ago

LGTM. My only concern is how do we ensure the tests can pass in java 8? We still want to keep java 8 supported

We want to keep java 8 support, so we keep the quickstart test for java 8, but no unit tests or integration tests.

Also, this setup blocks using the code feature of java 11.

If we use the code feature of java 11, we can no longer support compiling in java 8 right?

snleee commented 2 years ago

@Jackie-Jiang @xiangfu0 As Jackie pointed out, we cannot compile the code with Java 8 once we start to use java 11 features. Let's announce that we will officially deprecate the java 8 support from 0.8.0 release. After we cut that release, we can start to use java 11 features. Until then, let's not introduce java 11 feature to the codebase. Let's discuss if there are any immediate needs for java 11 features.

xiangfu0 commented 2 years ago

FYI: @yupeng9 @chenboat do you guys have any thoughts?

chenboat commented 2 years ago

FYI: @yupeng9 @chenboat do you guys have any thoughts?

we are in the process of releasing 0.7 now. if java 11 is used from 0.8 onward, we will do some test on our end before our next release.

xiangfu0 commented 2 years ago

FYI: @yupeng9 @chenboat do you guys have any thoughts?

we are in the process of releasing 0.7 now. if java 11 is used from 0.8 onward, we will do some test on our end before our next release.

We will keep the pinot 0.8 release still java8 compilable, then drop the support.

snleee commented 2 years ago

@chenboat the most recently released version is 0.7.1. The next release will be 0.8.0. So, we support java 8 compatible up to 0.8.0 and then drop the support moving forward. How does that sound? Also, is your runtime env using jvm 8 or jvm 11?

xiangfu0 commented 2 years ago

Discussed with @mcvsubbu @siddharthteotia @jackjlli offline, we will keep the JDK 8 support longer for LinkedIn to test and validate their deployment and data ingestion workflow.

The concrete date of dropping support for JDK8 is still TBD. @snleee

apache / pinot

Make Pinot JDK 11 Compilable #6424

Description

Upgrade Notes

Release Notes

Documentation

Codecov Report