[AMORO-3853] Support Java 17 build for Amoro#4244
Conversation
Add JDK 17 compatibility
…istency in time calculations
|
Could you please take some time to review this PR? This PR is a crucial prerequisite for upgrading Iceberg 1.11.0 and adapting to Iceberg v3. Thank you! cc @zhoujinsong @czy006 @klion26 @j1wonpark |
There was a problem hiding this comment.
Additional review notes to help focus this PR:
- The most regression-sensitive Flink sink path is the Iceberg files committer creation. Please keep it on the authenticated overload,
IcebergClassUtil.newIcebergFilesCommitter(..., mixedTable.io()), so committer callbacks still run throughAuthenticatedFileIO#doAsin Kerberos deployments. - When rebuilding Iceberg
ScanContextwithout the previous private/reflection-based path, split locality should be preserved with the same public behavior: honorFlinkConfigOptions.TABLE_EXEC_ICEBERG_EXPOSE_SPLIT_LOCALITY_INFO, and otherwise rely on Hadoop block-location availability through Iceberg public Hadoop utility. - For Hadoop3 Spark CI, JDK 17 should be treated as experimental coverage and only needs to run with Spark 3.5. Spark 3.3/3.4 remain covered by the JDK 11 matrix.
|
Thanks for the great contribution! I'll take a look at it this week. |
|
|
||
| private final MailboxExecutor executor; | ||
| private FlinkInputFormat format; | ||
| private transient SourceFunction.SourceContext<RowData> sourceContext; |
There was a problem hiding this comment.
Do we need to migrate to the new source api?
There was a problem hiding this comment.
UnkeyedStreamingReaderOperator is intentionally a minimal replacement for Iceberg's non-public StreamingReaderOperator, so that we can avoid depending on private Iceberg internals while preserving the existing FlinkInputFormat scan behavior and the Kerberos AuthenticatedFileIO#doAs wrapper.
| authenticatedFileIO.doAs( | ||
| () -> { | ||
| try { | ||
| method.setAccessible(true); |
There was a problem hiding this comment.
KerberosInvocationHandler currently proxies methods exposed through the proxied object's interfaces.
In practice, the invocation path goes through public interfaces such as OneInputStreamOperator and SourceFunction. As a result, these method invocations do not require method.setAccessible(true).
Keeping method.setAccessible(true) would introduce unnecessary deep-reflection behavior, which is exactly what this JDK17 compatibility change is trying to avoid.
j1wonpark
left a comment
There was a problem hiding this comment.
Built all modules on JDK 17, and ran the amoro-mixed-flink-common suite on both JDK 11 and 17 — identical results, no correctness issues found (the only failures are local Kafka testcontainer issues, unrelated to this PR).
…pt nullable startSnapshotId
Why are the changes needed?
Close #3853.
Amoro currently cannot reliably build and validate under JDK 17 beyond the Trino-specific path. Some build configuration still assumes older JDK behavior, and the mixed Flink module uses reflective access or non-public Flink/Iceberg internals that are fragile under the Java module system.
This PR adds experimental JDK 17 build support while keeping JDK 11 as the baseline, and updates CI to validate both JDK versions.
Brief change log
java17Maven profile with the required compiler, surefire, and javadoc module opens/exports.cglib,slf4j, surefire configuration, Hudi compiler inheritance, Paimon surefire setup, and Trino toolchain matching.KerberosAwareInputFormat;StreamingReaderOperatordependency withUnkeyedStreamingReaderOperator;FlinkClassReflectionUtil;FlinkInputFormatdirectly from public table metadata;How was this patch tested?
Add some test cases that check the changes thoroughly including negative and positive cases if possible
Existing mixed Flink tests were adjusted to cover the updated JDK 17-compatible read/write paths.
Add screenshots for manual tests if appropriate
Not applicable.
Run test locally before making a pull request
./mvnw validatepassed locally with Temurin JDK 17.0.17.Documentation
Does this pull request introduce a new feature? (yes / no)
yes
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
README