[infra] Derive and validate the Flink versions tools/ut.sh uses - #1007
Open
weiqingy wants to merge 3 commits into
Open
[infra] Derive and validate the Flink versions tools/ut.sh uses#1007weiqingy wants to merge 3 commits into
weiqingy wants to merge 3 commits into
Conversation
… pom The help text said that omitting -f runs every supported Flink version. It runs one. DEFAULT_FLINK_VERSION was pinned to 2.2 while the root pom carried 2.3.0, so a bare tools/ut.sh tested Java on 2.3.0 and Python on 2.2.x in the same command. The default now derives from <flink.version> in the root pom, so both lanes land on the same version and a bump cannot leave this script behind. tools/update_flink_version.sh rewrites poms only, which is how the two drifted apart in the first place. A value that is not shaped x.y is fatal rather than flowing on into dist module paths, -P profile names and the pip requirement. This changes what the Python unit-test jobs install, from apache-flink~=2.2.0 to ~=2.3.0, in both ci.yml and build_wheel.yml. Measured on the derived default: 825 passed, 13 skipped against PyFlink 2.3.0. The integration matrices pass -e -f explicitly and are unaffected. The help text now states the derived default, and that -f applies to the e2e and Python tests rather than to the Java unit tests. Generated-by: Claude Code 2.1.229 (Claude Opus 5)
tools/ut.sh -j -f 1.20 accepted the flag and then tested exactly what it would have tested anyway. The Java unit-test invocations pass no -P and exclude the e2e modules, so an explicitly requested version never reaches them, and nothing said so. The warning fires only when a version was passed rather than defaulted, Java is selected, and -e is not. It names the Java unit tests rather than claiming the flag was ignored outright, because a default run also executes the Python tests, which do install against the requested version. A warning rather than a rejection: rejecting the flag would turn a working command into a failing one, and the same information fits in a warning. None of the workflow invocations trigger it. Both integration matrices pass -e alongside -f, and the single-version jobs pass no -f. Generated-by: Claude Code 2.1.229 (Claude Opus 5)
2 tasks
-f took any string. tools/ut.sh -j -e -f 9.9 asked Maven to build dist/flink-9.9, a module that does not exist, and to activate a profile of the same name. A typo looked like it worked. The supported set now comes from the dist/flink-* modules rather than a second hardcoded list, and both the explicit value and the derived default are checked against it, so the two derivations cannot drift apart. Each rejection names its own cause: a caller error, a root pom that has moved ahead of the dist modules, or no dist modules at all. -f 2.3 also selected a profile that did not exist. The e2e integration pom defined flink-1.20 through flink-2.2 while 2.3 was its unprofiled default, so Maven warned and carried on, landing on 2.3.0 by fallthrough. Adding the profile makes the flag mean something for every value now accepted. The new profile omits flink.log4j2.version. Its four siblings pin 2.24.3 because they need a value different from the parent's; 2.3 needs the parent's 2.25.3, and a redundant pin would stop tracking it. Verified behavior-neutral: flink.version, flink.agents.dist.artifactId and flink.log4j2.version all resolve identically with and without -Pflink-2.3. A test asserts the three lists that must agree still do: the dist/flink-* directories, dist/pom.xml's modules, and the e2e integration pom's flink-* profile ids. Their disagreement is what left 2.3 without a profile. Generated-by: Claude Code 2.1.229 (Claude Opus 5)
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked issue: #1006
Purpose of change
tools/ut.shtold the reader that omitting-ftests every supported Flink version. It tests one, and on the Java unit-test path the selected version is not used at all. BecauseDEFAULT_FLINK_VERSIONwas pinned to2.2while the root pom carries2.3.0, a bare./tools/ut.shtested Java on 2.3.0 and Python on 2.2.x in the same command.The first commit derives the default from
<flink.version>in the root pom instead of pinning it, so both lanes land on the same version and the value cannot drift again. That drift is the root cause rather than an accident:tools/update_flink_version.shrewrites poms only and never touches this script, so the pinned literal fell a minor line behind. The derived value is validated to be shapedx.ybefore use, because it flows unvalidated intodist/flink-<v>paths,-Pflink-<v>profile names and theapache-flink~=<v>.0requirement. Reading the property withsedfollowstools/update_flink_version.sh, which already reads the same property the same way.The second commit warns when
-fcannot affect what was selected../tools/ut.sh -j -f 1.20previously accepted the flag and then tested exactly what it would have anyway. A warning was chosen over rejecting the flag, since rejecting it turns a currently-working command into a failing one.The third commit validates the versions
-faccepts. It took any string, so-f 9.9asked Maven to builddist/flink-9.9, a module that does not exist, and a typo looked like it worked. The supported set is derived from thedist/flink-*modules rather than a second hardcoded list, and the derived default is checked against it too, so the two derivations cannot drift apart. It also adds aflink-2.3profile to the e2e integration pom: that pom definedflink-1.20throughflink-2.2while 2.3 was its unprofiled default, so-f 2.3passed a profile Maven warned about and ignored, landing on 2.3.0 by fallthrough.The new profile deliberately omits
flink.log4j2.version. Its four siblings pin2.24.3because they need a value different from the parent's, while 2.3 needs the parent's2.25.3, so a redundant pin would stop tracking it on the next bump. Verified behavior-neutral:flink.version,flink.agents.dist.artifactIdandflink.log4j2.versionall resolve identically with and without-Pflink-2.3.This subsumes #1008, which was filed for those two defects and then closed as completed once the work moved here.
The help text now states the derived default and the flag's real scope, and its supported-version list is derived rather than a literal.
One option in the issue is deliberately not taken. Making the default iterate every supported version is not implementable today:
runtimefailstest-compileat 1.20 with 5 errors inRescalingTest.java, which importsorg.apache.flink.streaming.api.functions.sink.legacyand...source.legacy, packages that exist only in Flink 2.x. It would also multiply every job that runs the Java unit tests by roughly five, to duplicate coverage thejava_it_testsmatrix already provides across all five versions.Tests
New
tools/test/unit/ut_sh_version_default.bats(8 cases) andtools/test/unit/ut_sh_flink_flag_scope.bats(6 cases), in the existing bats harness that CI already runs. Both use the subprocess and shim patterns fromtools/test/integration/help.batsanddry_run.bats; neither sourcesut.sh, which has no run guard.The version cases cover the derived default, that it follows a changed pom rather than coincidentally matching today's value, whitespace-padded XML, and the three malformed shapes that must be fatal. The scope cases pin the firing condition rather than just the message, so an over-broad or under-broad condition fails.
A third file covers validation: that an unsupported
-fis rejected, that the supported set follows a fakedist/rather than being pinned to today's tree, and that each rejection names its own cause. One case is a drift guard asserting the three lists that must agree still do — thedist/flink-*directories,dist/pom.xml's<modules>, and the e2e integration pom'sflink-*profile ids. Their disagreement is what left 2.3 without a profile in the first place.Full harness: 212 ok / 1 not ok. The single failure is pre-existing and unrelated, in
verify_example_job.batsviae2e-test/test-scripts/test_submit_examples_to_flink.sh:362(remaining_pids[@]: unbound variable); it reproduces identically on unmodifiedmain../tools/ut.sh -pwas run on the new default: 825 passed, 13 skipped, exit 0, againstapache-flink 2.3.0confirmed from the environment rather than the log.Note one behavior change: the Python unit-test jobs in
ci.ymlandbuild_wheel.ymlnow installapache-flink~=2.3.0rather than~=2.2.0. PyFlink 2.3.0 is published and is the current latest on PyPI. Both integration matrices pass-e -fexplicitly and are unaffected, and none of the six workflow invocations emits the new warning.Not covered: the
pipfallback branch ofut.sh -pwas not exercised on the new default, only theuvbranch that CI uses.API
No public API change. The observable change is to the script's CLI behavior and help text, described above.
Documentation
doc-neededdoc-not-neededdoc-includedEvery inaccurate claim was in the script's own help text, which this PR corrects. No file under
docs/referencestools/ut.sh.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code 2.1.229 (Claude Opus 5)