-
Notifications
You must be signed in to change notification settings - Fork 2.5k
[HUDI-6509] Add GitHub CI for Java 17 #9136
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
Conversation
cb10175 to
8ea8e3a
Compare
|
@hudi-bot run azure |
a038fd6 to
c79f37c
Compare
|
It seems Need to look into this, otherwise everything looks good. newly added docker-test-java17 and test-spark-java17 are working fine |
.github/workflows/bot.yml
Outdated
| run: | ||
| mvn test -Pfunctional-tests -Pjava17 -D"$SCALA_PROFILE" -D"$SPARK_PROFILE" -pl "$SPARK_COMMON_MODULES,$SPARK_MODULES" $MVN_ARGS | ||
|
|
||
| docker-test-java17: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be run with validate-bundles since it already validates bundles on Java 17? Any reason to have a separate job here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
those tests are generally the same as bundle validation but there is one difference: It requires building Hudi within Docker as it runs tests with mvn test command, while bundle validation build Hudi outside of Docker and only copy jars/bundles to Docker. If we consolidates them as one then the job would need to build twice, it would make the job much slower.
If we just seperate them into 2 jobs then docker test can only build hudi-common modules in Docker which is relatively fast and bundle validation can keep the same behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to have one job and docker setup so it's easier to maintain. We can fix the repeated mvn build later on.
hudi-common/src/test/java/org/apache/hudi/common/fs/TestHoodieWrapperFileSystem.java
Outdated
Show resolved
Hide resolved
hudi-common/src/test/java/org/apache/hudi/common/functional/TestHoodieLogFormat.java
Outdated
Show resolved
Hide resolved
...common/src/test/java/org/apache/hudi/common/functional/TestHoodieLogFormatAppendFailure.java
Outdated
Show resolved
Hide resolved
| # - <dataset name>/schema.avsc | ||
| # - <dataset name>/data/<data files> | ||
| ################################################################################################# | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we consolidate the test logic of this into validate.sh and reuse existing validate-bundle job?
| <flink.connector.kafka.artifactId>flink-connector-kafka</flink.connector.kafka.artifactId> | ||
| <flink.hadoop.compatibility.artifactId>flink-hadoop-compatibility_2.12</flink.hadoop.compatibility.artifactId> | ||
| <rocksdbjni.version>5.17.2</rocksdbjni.version> | ||
| <rocksdbjni.version>7.5.3</rocksdbjni.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does RocksDB 5.17.2 not work? Dependency version upgrade has larger impact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, RocksDB 5.17.2 would throw NoClassDefException when running TestHoodieLogFormat
[ERROR] testBasicAppendAndScanMultipleFiles{DiskMapType, boolean, boolean, boolean}[10] Time elapsed: 0.118 s <<< ERROR!
2023-07-13T23:41:36.1420947Z java.lang.NoClassDefFoundError: Could not initialize class org.rocksdb.DBOptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fishy. Does this only happen for Java 17?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for Java 8 it works fine. This issue only pops up for Java 17
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reading the code, RocksDbBasedFileSystemView is the only place using RocksDb. So this should be ok.
95b9ca4 to
9bc5072
Compare
This reverts commit 68f6b69.
yihua
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for adding validations on Java 17!
| SCALA_PROFILE: ${{ matrix.scalaProfile }} | ||
| SPARK_PROFILE: ${{ matrix.sparkProfile }} | ||
| SPARK_MODULES: ${{ matrix.sparkModules }} | ||
| if: ${{ !endsWith(env.SPARK_PROFILE, '3.2') }} # skip test spark 3.2 as it's covered by Azure CI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: not required.
| SCALA_PROFILE: ${{ matrix.scalaProfile }} | ||
| SPARK_PROFILE: ${{ matrix.sparkProfile }} | ||
| SPARK_MODULES: ${{ matrix.sparkModules }} | ||
| if: ${{ !endsWith(env.SPARK_PROFILE, '3.2') }} # skip test spark 3.2 as it's covered by Azure CI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: not required.
| SPARK_PROFILE: ${{ matrix.sparkProfile }} | ||
| SPARK_RUNTIME: ${{ matrix.sparkRuntime }} | ||
| SCALA_PROFILE: 'scala-2.12' | ||
| if: ${{ env.SPARK_PROFILE >= 'spark3.4' }} # Only support Spark 3.4 for now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: not required.
| if [[ ${SPARK_RUNTIME} == 'spark2.4.8' ]]; then | ||
| HADOOP_VERSION=2.7.7 | ||
| HIVE_VERSION=2.3.9 | ||
| DERBY_VERSION=10.10.2.0 | ||
| FLINK_VERSION=1.13.6 | ||
| SPARK_VERSION=2.4.8 | ||
| SPARK_HADOOP_VERSION=2.7 | ||
| CONFLUENT_VERSION=5.5.12 | ||
| KAFKA_CONNECT_HDFS_VERSION=10.1.13 | ||
| IMAGE_TAG=flink1136hive239spark248 | ||
| elif [[ ${SPARK_RUNTIME} == 'spark3.0.2' ]]; then | ||
| HADOOP_VERSION=2.7.7 | ||
| HIVE_VERSION=3.1.3 | ||
| DERBY_VERSION=10.14.1.0 | ||
| FLINK_VERSION=1.14.6 | ||
| SPARK_VERSION=3.0.2 | ||
| SPARK_HADOOP_VERSION=2.7 | ||
| CONFLUENT_VERSION=5.5.12 | ||
| KAFKA_CONNECT_HDFS_VERSION=10.1.13 | ||
| IMAGE_TAG=flink1146hive313spark302 | ||
| elif [[ ${SPARK_RUNTIME} == 'spark3.1.3' ]]; then | ||
| HADOOP_VERSION=2.7.7 | ||
| HIVE_VERSION=3.1.3 | ||
| DERBY_VERSION=10.14.1.0 | ||
| FLINK_VERSION=1.13.6 | ||
| SPARK_VERSION=3.1.3 | ||
| SPARK_HADOOP_VERSION=2.7 | ||
| CONFLUENT_VERSION=5.5.12 | ||
| KAFKA_CONNECT_HDFS_VERSION=10.1.13 | ||
| IMAGE_TAG=flink1136hive313spark313 | ||
| elif [[ ${SPARK_RUNTIME} == 'spark3.2.3' ]]; then | ||
| HADOOP_VERSION=2.7.7 | ||
| HIVE_VERSION=3.1.3 | ||
| DERBY_VERSION=10.14.1.0 | ||
| FLINK_VERSION=1.14.6 | ||
| SPARK_VERSION=3.2.3 | ||
| SPARK_HADOOP_VERSION=2.7 | ||
| CONFLUENT_VERSION=5.5.12 | ||
| KAFKA_CONNECT_HDFS_VERSION=10.1.13 | ||
| IMAGE_TAG=flink1146hive313spark323 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: older Spark versions can be removed.
| <flink.connector.kafka.artifactId>flink-connector-kafka</flink.connector.kafka.artifactId> | ||
| <flink.hadoop.compatibility.artifactId>flink-hadoop-compatibility_2.12</flink.hadoop.compatibility.artifactId> | ||
| <rocksdbjni.version>5.17.2</rocksdbjni.version> | ||
| <rocksdbjni.version>7.5.3</rocksdbjni.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reading the code, RocksDbBasedFileSystemView is the only place using RocksDb. So this should be ok.
|
CI has passed for 6b33d37. No need to rerun CI again. |

Change Logs
Add a new GitHub action to test Hudi spark against Java 17. Some known failures would be excluded as they failed due to Hadoop dependencies like MiniDFSCluster
Impact
None
Risk level (write none, low medium or high below)
None
Documentation Update
Describe any necessary documentation update if there is any new feature, config, or user-facing change
ticket number here and follow the instruction to make
changes to the website.
Contributor's checklist