[runners-flink] Use index-based version comparison in flink_runner.gradle#38272
[runners-flink] Use index-based version comparison in flink_runner.gradle#38272tkaymak wants to merge 4 commits intoapache:masterfrom
Conversation
…adle
`previous_versions` was computed by lexicographic string comparison
against `flink_major`:
def previous_versions = all_versions.findAll { it < flink_major }
This breaks for `flink_versions=1.17,1.18,1.19,1.20,2.0` (the current
gradle.properties value) because lexicographic ordering disagrees with
semantic ordering whenever a two-digit minor crosses a single-digit
boundary. Concretely, today this resolves to:
flink_major=1.17 -> [] (correct)
flink_major=1.18 -> ["1.17", "1.20"] (wrong; should be ["1.17"])
flink_major=1.19 -> ["1.17", "1.18", "1.20"] (wrong; should be ["1.17", "1.18"])
flink_major=1.20 -> [] (wrong; should be ["1.17", "1.18", "1.19"])
flink_major=2.0 -> ["1.17", "1.18", "1.19", "1.20"](correct)
The 1.18 and 1.19 builds therefore pick up
`runners/flink/1.20/src/main/java/.../DoFnOperator.java` as a "previous"
override, and the 1.20 build loses the 1.19 test overrides
(MemoryStateBackendWrapper, StreamSources).
Replace the lex compare with index lookup against `flink_versions`,
exactly like spark_runner.gradle does (PR apache#38233). Also:
- Trim whitespace from each entry (defensive against
`flink_versions=1.17, 1.18, ...`).
- Throw a clear GradleException if `flink_major` isn't listed in
`flink_versions`, instead of silently producing an empty
`previous_versions` list.
- Update `use_override` to derive from the same index, mirroring the
Spark version.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the Flink runner's build configuration where version comparison was performed using string-based logic. This caused incorrect identification of previous versions when minor versions reached double digits, leading to erroneous source code overrides during compilation. The changes align the Flink runner's build logic with the Spark runner's approach by utilizing list indices for version ordering, ensuring build stability across different Flink versions. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
tkaymak
changed the title
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the Flink runner's Gradle build script to use list indices for version ordering instead of string comparisons, which correctly handles two-digit minor versions. The review feedback recommends improving the robustness of the version lookup by trimming the input version string and simplifying the code by removing a redundant ternary operator when calculating previous versions.
- Trim/stringify flink_major before indexOf so command-line / GString values match the trimmed all_versions entries. - Drop redundant ternary on previous_versions; subList(0, 0) is already empty when flink_major is the first listed version.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the Flink runner's Gradle configuration to correctly handle version ordering. It replaces string-based version comparisons with index-based lookups from the flink_versions list, ensuring that two-digit minor versions (e.g., 1.20) are correctly identified relative to earlier versions. Additionally, it adds validation to ensure the specified flink_major version exists in the configuration. I have no feedback to provide.
|
assign set of reviewers |
|
Assigning reviewers: R: @damccorm added as fallback since no labels match configuration Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
@Abacn would you mind taking a look at this one since you have context? |
2 participants
Summary
Promised follow-up to @Abacn from PR #38233 review (issue comment 2026-04-17), where the same lex-comparison bug was flagged in
spark_runner.gradleand asked to be split into a separate PR for the Flink side.runners/flink/flink_runner.gradleresolvesprevious_versions(the listof older Flink majors whose source-overrides should be merged into the
current build) via lexicographic string comparison against
flink_major:This breaks for the current
flink_versions=1.17,1.18,1.19,1.20,2.0becauselexicographic ordering disagrees with semantic ordering whenever a two-digit
minor crosses a single-digit boundary.
Impact today
flink_majorprevious_versions1.17[][]✓1.18["1.17", "1.20"]❌["1.17"]1.19["1.17", "1.18", "1.20"]❌["1.17", "1.18"]1.20[]❌["1.17", "1.18", "1.19"]2.0["1.17", "1.18", "1.19", "1.20"]Concretely:
runners/flink/1.20/src/main/java/.../DoFnOperator.javaas a "previous" override. If that file relies on Flink 1.20+ APIs, those builds would fail to compile.MemoryStateBackendWrapper,StreamSources). Both classes also exist in the shared base, so 1.20's tests use the shared-base versions today; whether that's correct depends on whether the 1.19 overrides exist precisely because the shared-base versions don't compile against the 1.19+ test API.Fix
Mirror the Spark approach landed in #38233 (
runners/spark/spark_runner.gradle):flink_major_index = all_versions.indexOf(flink_major)and deriveprevious_versionsasall_versions.subList(0, flink_major_index).GradleExceptionifflink_majorisn't listed inflink_versions(today the buggy path silently produced[]).use_overrideto(flink_major_index > 0), matching the Spark formulation.flink_versionsentry, defensive againstflink_versions=1.17, 1.18, ...style.