Make Pinot JDK 11 Compilable #6424

elonazoulay · 2021-01-08T09:24:04Z

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

pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginClassLoader.java

xiangfu0 · 2021-02-11T20:43:40Z

delete this gc.log.*?

kubernetes/helm/pinot/values.yaml

Jackie-Jiang · 2021-02-17T20:20:03Z

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

elonazoulay · 2021-04-28T12:34:12Z

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 · 2021-04-28T16:29:28Z

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%)`	⬇️
unittests	`65.50% <100.00%> (+0.10%)`	⬆️

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%)`	⬆️
...ot/segment/local/customobject/MinMaxRangePair.java	`75.86% <0.00%> (-24.14%)`	⬇️
...g/apache/pinot/segment/spi/memory/CleanerUtil.java	`33.87% <0.00%> (-23.28%)`	⬇️
...n/src/main/java/org/apache/pinot/common/Utils.java	`40.42% <0.00%> (-19.15%)`	⬇️
...e/pinot/core/transport/InstanceRequestHandler.java	`62.31% <0.00%> (-16.26%)`	⬇️
.../apache/pinot/core/transport/DataTableHandler.java	`88.00% <0.00%> (-12.00%)`	⬇️
...pache/pinot/core/query/utils/idset/EmptyIdSet.java	`25.00% <0.00%> (-10.72%)`	⬇️
...re/operator/dociditerators/EmptyDocIdIterator.java	`75.00% <0.00%> (-8.34%)`	⬇️
...inion/event/DefaultMinionEventObserverFactory.java	`66.66% <0.00%> (-8.34%)`	⬇️
...impl/dictionary/DoubleOnHeapMutableDictionary.java	`46.98% <0.00%> (-6.03%)`	⬇️
... 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 · 2021-04-28T21:59:52Z

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

+100 for this change!

xiangfu0 · 2021-04-28T23:01:08Z

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.

The application classloader does not extend URLClassLoader in java9+. See https://community.oracle.com/tech/developers/discussion/4011800/base-classloader-no-longer-from-urlclassloader. By invoking addURL directly, if the class is not found in the delegate classLoader it will be found via findClass().

Add _complexFieldSpecs to equals and hashCode. Update MetadataEqualsHashCodeTest to ignore _virtualColumnProvider in equality/hashcode tests.

IOException is no longer a checked exception of ZkClient.readData. ZkClient.readData throws an unchecked ZkNoNodeException which extends RuntimeException.

The test works locally but in ci it fails sporadically.

This test passes locally but fails sporadically in ci.

Jackie-Jiang

Do we want to consider running one test suite with java 8 and another one with java 11 to ensure both work as expected?

...-clients/pinot-java-client/src/test/java/org/apache/pinot/client/ExternalViewReaderTest.java

Jackie-Jiang · 2021-06-09T23:23:50Z

pinot-common/src/test/java/org/apache/pinot/common/utils/PinotDataTypeTest.java

@@ -105,6 +105,7 @@ public void testBytes() {
  @Test
  public void testTimestamp() {
    Timestamp timestamp = Timestamp.valueOf(LocalDateTime.now());
+    timestamp.setNanos(0);


Is this related?

yes, java11 uses nano by default, Pinot converter doesn't keep the nano part.

In that case, maybe cleaner if directly initialize with millis

Suggested change

timestamp.setNanos(0);

Timestamp timestamp = new Timestamp(System.currentTimeMillis());

pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginClassLoader.java

Jackie-Jiang · 2021-06-09T23:33:59Z

pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginClassLoader.java

    for (URL url : urls) {
      try {
-        method.invoke(classLoader, url);
+        /**
+         * ClassLoader in java9+ does not extend URLClassLoader.


Don't quite understand the comment here. Do you mean URLClassLoader does not extend ClassLoader in java 9? But it should still have the method addURL() right?

URLClassLoader still extend ClassLoader and this addURL method is there.
Actually, this part also works for java 8.
@kishoreg @elonazoulay thoughts?

kubernetes/helm/pinot/values.yaml

Jackie-Jiang · 2021-06-10T16:19:50Z

pinot-common/src/test/java/org/apache/pinot/common/utils/PinotDataTypeTest.java

@@ -105,6 +105,7 @@ public void testBytes() {
  @Test
  public void testTimestamp() {
    Timestamp timestamp = Timestamp.valueOf(LocalDateTime.now());
+    timestamp.setNanos(0);


In that case, maybe cleaner if directly initialize with millis

Suggested change

timestamp.setNanos(0);

Timestamp timestamp = new Timestamp(System.currentTimeMillis());

Jackie-Jiang

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

xiangfu0 · 2021-06-11T18:33:18Z

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 · 2021-06-11T18:45:11Z

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 · 2021-06-11T20:15:15Z

@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 · 2021-06-11T20:16:54Z

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

chenboat · 2021-06-11T20:34:34Z

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 · 2021-06-11T20:48:06Z

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 · 2021-06-11T21:12:31Z

@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 · 2021-06-14T20:44:51Z

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

* Fix PluginClassLoader when using JRE9+ The application classloader does not extend URLClassLoader in java9+. See https://community.oracle.com/tech/developers/discussion/4011800/base-classloader-no-longer-from-urlclassloader. By invoking addURL directly, if the class is not found in the delegate classLoader it will be found via findClass(). * Fix Schema.equals Add _complexFieldSpecs to equals and hashCode. Update MetadataEqualsHashCodeTest to ignore _virtualColumnProvider in equality/hashcode tests. * Fix ExternalViewReaderTest IOException is no longer a checked exception of ZkClient.readData. ZkClient.readData throws an unchecked ZkNoNodeException which extends RuntimeException. * Update maven compiler to use java11 * Remove internal jdk apis from appAssemblerScriptTemplate * use different jdk.version in quickstart * Make SegmentDeletionManagerTest single threaded The test works locally but in ci it fails sporadically. * Increase wait for scheduled tasks in BasicAuthRealtimeIntegrationTest This test passes locally but fails sporadically in ci. * fixup! Fix Schema.equals * fixing tests Co-authored-by: Xiang Fu <xiangfu.1024@gmail.com>

xiangfu0 requested review from xiangfu0, kishoreg, Jackie-Jiang and mcvsubbu February 11, 2021 20:41

xiangfu0 reviewed Feb 11, 2021

View reviewed changes

pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginClassLoader.java Outdated Show resolved Hide resolved

xiangfu0 reviewed Feb 11, 2021

View reviewed changes

kubernetes/helm/pinot/values.yaml Outdated Show resolved Hide resolved

xiangfu0 mentioned this pull request Mar 17, 2021

Upgrade Pinot JDK Support to 11 #6689

Closed

4 tasks

elonazoulay force-pushed the java11 branch 2 times, most recently from f319079 to 7814776 Compare April 28, 2021 12:32

elonazoulay force-pushed the java11 branch 5 times, most recently from 9d5c69b to 01ddd82 Compare April 28, 2021 15:20

xiangfu0 added backward-incompat Referenced by PRs that introduce or fix backward compat issues incompatible Indicate PR that introduces backward incompatibility release-notes Referenced by PRs that need attention when compiling the next release notes labels Apr 28, 2021

xiangfu0 approved these changes Apr 28, 2021

View reviewed changes

xiangfu0 requested review from xiangfu0, jackjlli, KKcorps, mayankshriv, siddharthteotia and snleee April 28, 2021 23:09

elonazoulay and others added 9 commits June 7, 2021 12:24

Fix Schema.equals

19c4fba

Add _complexFieldSpecs to equals and hashCode. Update MetadataEqualsHashCodeTest to ignore _virtualColumnProvider in equality/hashcode tests.

Fix ExternalViewReaderTest

64ea272

IOException is no longer a checked exception of ZkClient.readData. ZkClient.readData throws an unchecked ZkNoNodeException which extends RuntimeException.

Update maven compiler to use java11

25846e7

Remove internal jdk apis from appAssemblerScriptTemplate

e68726c

use different jdk.version in quickstart

edb70a3

Make SegmentDeletionManagerTest single threaded

54cbfb9

The test works locally but in ci it fails sporadically.

Increase wait for scheduled tasks in BasicAuthRealtimeIntegrationTest

acdb525

This test passes locally but fails sporadically in ci.

fixup! Fix Schema.equals

5e1326b

elonazoulay force-pushed the java11 branch from 4048f67 to 5e1326b Compare June 8, 2021 00:26

xiangfu0 force-pushed the java11 branch from b201f4c to 04e2c83 Compare June 9, 2021 05:48

xiangfu0 changed the title ~~Use java11~~ Make Pinot JDK 11 Compilable Jun 9, 2021

Jackie-Jiang reviewed Jun 9, 2021

View reviewed changes

xiangfu0 force-pushed the java11 branch from e365165 to 291d8f6 Compare June 10, 2021 01:16

Jackie-Jiang reviewed Jun 10, 2021

View reviewed changes

xiangfu0 force-pushed the java11 branch from 291d8f6 to dcc6faa Compare June 10, 2021 19:53

fixing tests

6b3ed47

xiangfu0 force-pushed the java11 branch from dcc6faa to 6b3ed47 Compare June 10, 2021 20:05

xiangfu0 requested a review from Jackie-Jiang June 11, 2021 04:45

Jackie-Jiang approved these changes Jun 11, 2021

View reviewed changes

xiangfu0 merged commit 4fe7153 into apache:master Jun 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Pinot JDK 11 Compilable #6424

Make Pinot JDK 11 Compilable #6424

elonazoulay commented Jan 8, 2021 •

edited by xiangfu0

xiangfu0 commented Feb 11, 2021

Jackie-Jiang commented Feb 17, 2021

elonazoulay commented Apr 28, 2021

codecov-commenter commented Apr 28, 2021 •

edited

xiangfu0 commented Apr 28, 2021

xiangfu0 left a comment

xiangfu0 commented Apr 28, 2021 •

edited

Jackie-Jiang left a comment

Jackie-Jiang Jun 9, 2021

xiangfu0 Jun 10, 2021

Jackie-Jiang Jun 10, 2021

Jackie-Jiang Jun 9, 2021

xiangfu0 Jun 10, 2021

Jackie-Jiang Jun 10, 2021

Jackie-Jiang left a comment

xiangfu0 commented Jun 11, 2021

Jackie-Jiang commented Jun 11, 2021

snleee commented Jun 11, 2021 •

edited

xiangfu0 commented Jun 11, 2021

chenboat commented Jun 11, 2021

xiangfu0 commented Jun 11, 2021 •

edited

snleee commented Jun 11, 2021

xiangfu0 commented Jun 14, 2021

	timestamp.setNanos(0);
	Timestamp timestamp = new Timestamp(System.currentTimeMillis());

Make Pinot JDK 11 Compilable #6424

Make Pinot JDK 11 Compilable #6424

Conversation

elonazoulay commented Jan 8, 2021 • edited by xiangfu0

Description

Upgrade Notes

Release Notes

Documentation

xiangfu0 commented Feb 11, 2021

Jackie-Jiang commented Feb 17, 2021

elonazoulay commented Apr 28, 2021

codecov-commenter commented Apr 28, 2021 • edited

Codecov Report

xiangfu0 commented Apr 28, 2021

xiangfu0 left a comment

Choose a reason for hiding this comment

xiangfu0 commented Apr 28, 2021 • edited

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Jackie-Jiang Jun 9, 2021

Choose a reason for hiding this comment

xiangfu0 Jun 10, 2021

Choose a reason for hiding this comment

Jackie-Jiang Jun 10, 2021

Choose a reason for hiding this comment

Jackie-Jiang Jun 9, 2021

Choose a reason for hiding this comment

xiangfu0 Jun 10, 2021

Choose a reason for hiding this comment

Jackie-Jiang Jun 10, 2021

Choose a reason for hiding this comment

Jackie-Jiang left a comment

Choose a reason for hiding this comment

xiangfu0 commented Jun 11, 2021

Jackie-Jiang commented Jun 11, 2021

snleee commented Jun 11, 2021 • edited

xiangfu0 commented Jun 11, 2021

chenboat commented Jun 11, 2021

xiangfu0 commented Jun 11, 2021 • edited

snleee commented Jun 11, 2021

xiangfu0 commented Jun 14, 2021

elonazoulay commented Jan 8, 2021 •

edited by xiangfu0

codecov-commenter commented Apr 28, 2021 •

edited

xiangfu0 commented Apr 28, 2021 •

edited

snleee commented Jun 11, 2021 •

edited

xiangfu0 commented Jun 11, 2021 •

edited