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

Make Pinot JDK 11 Compilable #6424

Merged
merged 10 commits into from
Jun 14, 2021
Merged
8 changes: 4 additions & 4 deletions .github/workflows/pinot_tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,10 @@ jobs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- name: Set up JDK 1.8
- name: Set up JDK 11
uses: actions/setup-java@v1
with:
java-version: 1.8
java-version: 11
- name: Unit Test
env:
RUN_INTEGRATION_TESTS: false
Expand All @@ -59,10 +59,10 @@ jobs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- name: Set up JDK 1.8
- name: Set up JDK 11
uses: actions/setup-java@v1
with:
java-version: 1.8
java-version: 11
- name: Integration Test
env:
RUN_INTEGRATION_TESTS: true
Expand Down
28 changes: 27 additions & 1 deletion .github/workflows/scripts/.pinot_quickstart.sh
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,38 @@ netstat -i

# Java version
java -version
jdk_version() {
IFS='
'
# remove \r for Cygwin
lines=$(java -Xms32M -Xmx32M -version 2>&1 | tr '\r' '\n')
for line in $lines; do
if test -z $result && echo "$line" | grep -q 'version "'
then
ver=$(echo $line | sed -e 's/.*version "\(.*\)"\(.*\)/\1/; 1q')
# on macOS, sed doesn't support '?'
if case $ver in "1."*) true;; *) false;; esac;
then
result=$(echo $ver | sed -e 's/1\.\([0-9]*\)\(.*\)/\1/; 1q')
else
result=$(echo $ver | sed -e 's/\([0-9]*\)\(.*\)/\1/; 1q')
fi
fi
done
unset IFS
echo "$result"
}
JAVA_VER="$(jdk_version)"

# Build
PASS=0
for i in $(seq 1 2)
do
mvn clean install -B -DskipTests=true -Pbin-dist -Dmaven.javadoc.skip=true
if [ "$JAVA_VER" -gt 11 ] ; then
mvn clean install -B -DskipTests=true -Pbin-dist -Dmaven.javadoc.skip=true -Djdk.version=11
else
mvn clean install -B -DskipTests=true -Pbin-dist -Dmaven.javadoc.skip=true -Djdk.version=${JAVA_VER}
fi
if [ $? -eq 0 ]; then
PASS=1
break;
Expand Down
2 changes: 1 addition & 1 deletion docker/images/pinot/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
# specific language governing permissions and limitations
# under the License.
#
ARG JAVA_VERSION=8
ARG JAVA_VERSION=11
FROM openjdk:${JAVA_VERSION} AS pinot_build_env

LABEL MAINTAINER=dev@pinot.apache.org
Expand Down
8 changes: 4 additions & 4 deletions kubernetes/helm/pinot/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ controller:
host: pinot-controller
port: 9000

jvmOpts: "-Xms256M -Xmx1G -XX:+UseG1GC -XX:MaxGCPauseMillis=200 -XX:+PrintGCDetails -XX:+PrintGCDateStamps -XX:+PrintGCApplicationStoppedTime -XX:+PrintGCApplicationConcurrentTime -Xloggc:/opt/pinot/gc-pinot-controller.log"
jvmOpts: "-Xms256M -Xmx1G -XX:+UseG1GC -XX:MaxGCPauseMillis=200 -XX:+PrintGCDetails -XX:+PrintGCDateStamps -XX:+PrintGCApplicationStoppedTime -XX:+PrintGCApplicationConcurrentTime -Xlog:gc*,safepoint:gc.log:time,uptime -Xlog:gc:/opt/pinot/gc-pinot-controller.log"

log4j2ConfFile: /opt/pinot/conf/log4j2.xml
pluginsDir: /opt/pinot/plugins
Expand Down Expand Up @@ -152,7 +152,7 @@ broker:
# fsGroup: 2000
securityContext: {}

jvmOpts: "-Xms256M -Xmx1G -XX:+UseG1GC -XX:MaxGCPauseMillis=200 -XX:+PrintGCDetails -XX:+PrintGCDateStamps -XX:+PrintGCApplicationStoppedTime -XX:+PrintGCApplicationConcurrentTime -Xloggc:/opt/pinot/gc-pinot-broker.log"
jvmOpts: "-Xms256M -Xmx1G -XX:+UseG1GC -XX:MaxGCPauseMillis=200 -XX:+PrintGCDetails -XX:+PrintGCDateStamps -XX:+PrintGCApplicationStoppedTime -XX:+PrintGCApplicationConcurrentTime -Xlog:gc*,safepoint:gc.log:time,uptime -Xlog:gc:/opt/pinot/gc-pinot-broker.log"

log4j2ConfFile: /opt/pinot/conf/log4j2.xml
pluginsDir: /opt/pinot/plugins
Expand Down Expand Up @@ -244,7 +244,7 @@ server:
storageClass: ""
#storageClass: "ssd"

jvmOpts: "-Xms512M -Xmx1G -XX:+UseG1GC -XX:MaxGCPauseMillis=200 -XX:+PrintGCDetails -XX:+PrintGCDateStamps -XX:+PrintGCApplicationStoppedTime -XX:+PrintGCApplicationConcurrentTime -Xloggc:/opt/pinot/gc-pinot-server.log"
jvmOpts: "-Xms512M -Xmx1G -XX:+UseG1GC -XX:MaxGCPauseMillis=200 -XX:+PrintGCDetails -XX:+PrintGCDateStamps -XX:+PrintGCApplicationStoppedTime -XX:+PrintGCApplicationConcurrentTime -Xlog:gc*,safepoint:gc.log:time,uptime -Xlog:gc:/opt/pinot/gc-pinot-server.log"

log4j2ConfFile: /opt/pinot/conf/log4j2.xml
pluginsDir: /opt/pinot/plugins
Expand Down Expand Up @@ -317,7 +317,7 @@ minion:
readinessEnabled: false

dataDir: /var/pinot/minion/data
jvmOpts: "-Xms256M -Xmx1G -XX:+UseG1GC -XX:MaxGCPauseMillis=200 -XX:+PrintGCDetails -XX:+PrintGCDateStamps -XX:+PrintGCApplicationStoppedTime -XX:+PrintGCApplicationConcurrentTime -Xloggc:/opt/pinot/gc-pinot-minion.log"
jvmOpts: "-Xms256M -Xmx1G -XX:+UseG1GC -XX:MaxGCPauseMillis=200 -XX:+PrintGCDetails -XX:+PrintGCDateStamps -XX:+PrintGCApplicationStoppedTime -XX:+PrintGCApplicationConcurrentTime -Xlog:gc*,safepoint:gc.log:time,uptime -Xlog:gc:/opt/pinot/gc-pinot-minion.log"

log4j2ConfFile: /opt/pinot/conf/log4j2.xml
pluginsDir: /opt/pinot/plugins
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ public void testGetLiveBrokersExceptionState() throws IOException {
// Setup
final List<String> expectedResult = Arrays.asList();
when(mockZkClient.readData(Mockito.anyString(), Mockito.anyBoolean())).thenThrow(
IOException.class);
RuntimeException.class);
Jackie-Jiang marked this conversation as resolved.
Show resolved Hide resolved

// Run the test
final List<String> result = externalViewReaderUnderTest.getLiveBrokers();
Expand Down Expand Up @@ -104,7 +104,7 @@ public void testGetTableToBrokersMapExceptionState() {
// Setup
final Map<String, List<String>> expectedResult = new HashMap<>();
when(mockZkClient.readData(Mockito.anyString(), Mockito.anyBoolean())).thenThrow(
IOException.class);
RuntimeException.class);

// Run the test
final Map<String, List<String>> result = externalViewReaderUnderTest.getTableToBrokersMap();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
package org.apache.pinot.common.utils;

import java.sql.Timestamp;
import java.time.LocalDateTime;
import java.util.Arrays;
import java.util.HashMap;
import java.util.Map;
Expand Down Expand Up @@ -104,7 +103,7 @@ public void testBytes() {

@Test
public void testTimestamp() {
Timestamp timestamp = Timestamp.valueOf(LocalDateTime.now());
Timestamp timestamp = new Timestamp(System.currentTimeMillis());
assertEquals(TIMESTAMP.convert(timestamp.getTime(), LONG), timestamp);
assertEquals(TIMESTAMP.convert(timestamp.toString(), STRING), timestamp);
assertEquals(TIMESTAMP.convert(timestamp.getTime(), JSON), timestamp);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
package org.apache.pinot.spi.plugin;

import java.io.IOException;
import java.lang.reflect.Method;
import java.net.URL;
import java.net.URLClassLoader;
import java.util.Enumeration;
Expand All @@ -36,18 +35,16 @@ public class PluginClassLoader extends URLClassLoader {
public PluginClassLoader(URL[] urls, ClassLoader parent) {
super(urls, parent);
classLoader = PluginClassLoader.class.getClassLoader();
Method method = null;
try {
method = URLClassLoader.class.getDeclaredMethod("addURL", URL.class);

} catch (NoSuchMethodException e) {
//this should never happen
ExceptionUtils.rethrow(e);
}
method.setAccessible(true);
for (URL url : urls) {
try {
method.invoke(classLoader, url);
/**
* ClassLoader in java9+ does not extend URLClassLoader.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* If the class is not found in the parent classloader,
* it will be found in this classloader via findClass().
*
* @see https://community.oracle.com/tech/developers/discussion/4011800/base-classloader-no-longer-from-urlclassloader
*/
addURL(url);
} catch (Exception e) {
ExceptionUtils.rethrow(e);
}
Expand Down
102 changes: 33 additions & 69 deletions pinot-tools/src/main/resources/appAssemblerScriptTemplate
Original file line number Diff line number Diff line change
Expand Up @@ -18,28 +18,6 @@
# under the License.
#

jdk_version() {
IFS='
'
# remove \r for Cygwin
lines=$(java -Xms32M -Xmx32M -version 2>&1 | tr '\r' '\n')
for line in $lines; do
if test -z $result && echo "$line" | grep -q 'version "'
then
ver=$(echo $line | sed -e 's/.*version "\(.*\)"\(.*\)/\1/; 1q')
# on macOS, sed doesn't support '?'
if case $ver in "1."*) true;; *) false;; esac;
then
result=$(echo $ver | sed -e 's/1\.\([0-9]*\)\(.*\)/\1/; 1q')
else
result=$(echo $ver | sed -e 's/\([0-9]*\)\(.*\)/\1/; 1q')
fi
fi
done
unset IFS
echo "$result"
}

# resolve links - $0 may be a softlink
PRG="$0"

Expand Down Expand Up @@ -129,46 +107,40 @@ if [ -n "$CLASSPATH_PREFIX" ] ; then
CLASSPATH=$CLASSPATH_PREFIX:$CLASSPATH
fi


JAVA_VER="$(jdk_version)"

# For java 9 and later version, we need to explicitly set Pinot Plugins directory into classpath.
if [ "$JAVA_VER" -gt 8 ] ; then
# Set $PLUGINS_CLASSPATH for plugin jars to be put into classpath.
# $PLUGINS_DIR and $PLUGINS_INCLUDE are used if $PLUGINS_CLASSPATH is not set.
# $PLUGINS_DIR is the root directory of plugins directory, default to '"$BASEDIR"/plugins' if not set.
# $PLUGINS_INCLUDE is semi-colon separated plugins name, e.g. pinot-avro;pinot-batch-ingestion-standalone. Default is not set, which means load all the plugin jars.
if [ -z "$PLUGINS_CLASSPATH" ] ; then
if [ -z "$PLUGINS_DIR" ] ; then
PLUGINS_DIR="$BASEDIR"/plugins
fi
if [ -d "$PLUGINS_DIR" ] ; then
if [ -n "$PLUGINS_INCLUDE" ] ; then
IFS=';' echo "$PLUGINS_INCLUDE" | read -ra PLUGINS_ARR
for PLUGIN_JAR in "${PLUGINS_ARR[@]}"; do
PLUGIN_JAR_PATH=$(find "$PLUGINS_DIR" -path \*/"$PLUGIN_JAR"/"$PLUGIN_JAR"-\*.jar)
if [ -n "$PLUGINS_CLASSPATH" ] ; then
PLUGINS_CLASSPATH=$PLUGINS_CLASSPATH:$PLUGIN_JAR_PATH
else
PLUGINS_CLASSPATH=$PLUGIN_JAR_PATH
fi
done
else
PLUGIN_JARS=$(find "$PLUGINS_DIR" -name \*.jar)
for PLUGIN_JAR in $PLUGIN_JARS ; do
# Set $PLUGINS_CLASSPATH for plugin jars to be put into classpath.
# $PLUGINS_DIR and $PLUGINS_INCLUDE are used if $PLUGINS_CLASSPATH is not set.
# $PLUGINS_DIR is the root directory of plugins directory, default to '"$BASEDIR"/plugins' if not set.
# $PLUGINS_INCLUDE is semi-colon separated plugins name, e.g. pinot-avro;pinot-batch-ingestion-standalone. Default is not set, which means load all the plugin jars.
if [ -z "$PLUGINS_CLASSPATH" ] ; then
if [ -z "$PLUGINS_DIR" ] ; then
PLUGINS_DIR="$BASEDIR"/plugins
fi
if [ -d "$PLUGINS_DIR" ] ; then
if [ -n "$PLUGINS_INCLUDE" ] ; then
IFS=';' echo "$PLUGINS_INCLUDE" | read -ra PLUGINS_ARR
for PLUGIN_JAR in "${PLUGINS_ARR[@]}"; do
PLUGIN_JAR_PATH=$(find "$PLUGINS_DIR" -path \*/"$PLUGIN_JAR"/"$PLUGIN_JAR"-\*.jar)
if [ -n "$PLUGINS_CLASSPATH" ] ; then
PLUGINS_CLASSPATH=$PLUGINS_CLASSPATH:$PLUGIN_JAR
PLUGINS_CLASSPATH=$PLUGINS_CLASSPATH:$PLUGIN_JAR_PATH
else
PLUGINS_CLASSPATH=$PLUGIN_JAR
PLUGINS_CLASSPATH=$PLUGIN_JAR_PATH
fi
done
fi
done
else
PLUGIN_JARS=$(find "$PLUGINS_DIR" -name \*.jar)
for PLUGIN_JAR in $PLUGIN_JARS ; do
if [ -n "$PLUGINS_CLASSPATH" ] ; then
PLUGINS_CLASSPATH=$PLUGINS_CLASSPATH:$PLUGIN_JAR
else
PLUGINS_CLASSPATH=$PLUGIN_JAR
fi
done
fi
fi
fi

if [ -n "$PLUGINS_CLASSPATH" ] ; then
CLASSPATH=$CLASSPATH:$PLUGINS_CLASSPATH
fi
if [ -n "$PLUGINS_CLASSPATH" ] ; then
CLASSPATH=$CLASSPATH:$PLUGINS_CLASSPATH
fi

# For Cygwin, switch paths to Windows format before running java
Expand All @@ -186,20 +158,12 @@ else
ALL_JAVA_OPTS=$JAVA_OPTS
fi

# For java 8, we set jvm system property `plugins.dir` to load Pinot plugins
if [ "$JAVA_VER" -eq 8 ] ; then
if [ -z "$PLUGINS_DIR" ] ; then
PLUGINS_DIR=$BASEDIR/plugins
fi
ALL_JAVA_OPTS="$ALL_JAVA_OPTS -Dplugins.dir=$PLUGINS_DIR"
if [ -n "$PLUGINS_INCLUDE" ] ; then
ALL_JAVA_OPTS="$ALL_JAVA_OPTS -Dplugins.include=$PLUGINS_INCLUDE"
fi
if [ -z "$PLUGINS_DIR" ] ; then
PLUGINS_DIR=$BASEDIR/plugins
fi

# For java 9 and later, we need to set extra java options to access JDK’s internal APIs.
if [ "$JAVA_VER" -gt 8 ] ; then
ALL_JAVA_OPTS="--add-exports java.base/jdk.internal.ref=ALL-UNNAMED $ALL_JAVA_OPTS"
ALL_JAVA_OPTS="$ALL_JAVA_OPTS -Dplugins.dir=$PLUGINS_DIR"
if [ -n "$PLUGINS_INCLUDE" ] ; then
ALL_JAVA_OPTS="$ALL_JAVA_OPTS -Dplugins.include=$PLUGINS_INCLUDE"
fi

exec "$JAVACMD" $ALL_JAVA_OPTS \
Expand Down
12 changes: 7 additions & 5 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@
<properties>
<pinot.root>${basedir}</pinot.root>
<build.profile.id>dev</build.profile.id>
<jdk.version>1.8</jdk.version>
<jdk.version>11</jdk.version>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<!-- Only unit tests are run by default. -->
<skip.integration.tests>true</skip.integration.tests>
Expand Down Expand Up @@ -151,7 +151,7 @@
<jts.version>1.16.1</jts.version>
<h3.version>3.0.3</h3.version>
<jmh.version>1.26</jmh.version>
<audienceannotations.version>0.11.0</audienceannotations.version>
<audienceannotations.version>0.13.0</audienceannotations.version>

<!-- Sets the VM argument line used when unit tests are run. -->
<argLine>-Xms4g -Xmx4g</argLine>
Expand Down Expand Up @@ -350,7 +350,7 @@
<dependency>
<groupId>nl.jqno.equalsverifier</groupId>
<artifactId>equalsverifier</artifactId>
<version>1.7.2</version>
<version>3.6</version>
<scope>test</scope>
</dependency>
<dependency>
Expand Down Expand Up @@ -1171,7 +1171,7 @@
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
<version>2.10.0</version>
<version>3.9.0</version>
<scope>test</scope>
</dependency>
<dependency>
Expand Down Expand Up @@ -1290,6 +1290,8 @@
<source>${jdk.version}</source>
<target>${jdk.version}</target>
<compilerVersion>${jdk.version}</compilerVersion>
<fork>true</fork>
<encoding>${project.build.sourceEncoding}</encoding>
</configuration>
</plugin>
<plugin>
Expand Down Expand Up @@ -1553,7 +1555,7 @@
<plugin>
<groupId>org.jacoco</groupId>
<artifactId>jacoco-maven-plugin</artifactId>
<version>0.7.7.201606060606</version>
<version>0.8.6</version>
<executions>
<execution>
<goals>
Expand Down