[FLINK-40551][tests] Select the checkpoint to restore by content in FileMergingChannelStateITCase - #29161
[FLINK-40551][tests] Select the checkpoint to restore by content in FileMergingChannelStateITCase#29161pedromazala wants to merge 1 commit into
Conversation
50926be to
6e8ff1a
Compare
6e8ff1a to
f120706
Compare
…ileMergingChannelStateITCase The test asserted that the checkpoint it restores from carries in-flight input channel state for the slow mapper, but selected that checkpoint by waiting for the second completed checkpoint that persisted any in-flight data anywhere in the job. Which subtask's buffers land in a given checkpoint depends on where the barriers are when it is triggered, so the count was a calibration rather than a condition: instrumenting the scan shows checkpoint 1 persists ~295 KB of in-flight data while carrying no input channel state for the mapper at all, and only checkpoint 2 has it on this hardware. Walk the completed checkpoint history instead and restore from the first checkpoint whose metadata really contains file-merged input channel state for the slow mapper. The lookup reports which checkpoints it inspected if the job terminates first, and skips checkpoints cleaned up while being read. The SegmentFileStateHandle and state size checks stay hard assertions on the selected checkpoint, so a file merging regression still fails loudly instead of timing out. Generated-by: Claude Opus 5 (1M context)
f120706 to
26cc91b
Compare
snuyanzin
left a comment
There was a problem hiding this comment.
Thank you for the contribution
Can you explain please what exactly you are fixing and why?
|
I'm asking since the FLINK-40551 was fixed by reverting original commit at after that it was not fail so why do we need to change anything else here? |
| final CheckpointMetadata metadata = | ||
| TestUtils.loadCheckpointMetadata(checkpoint.getExternalPath()); | ||
| return !collectChannelStateDelegates(metadata).slowMapperInputChannelState.isEmpty(); | ||
| } catch (IOException e) { |
There was a problem hiding this comment.
This catches more than the subsumption race described by the comment.loadCheckpointMetadata also reports corrupt headers, unsupported versions, truncated input, and deserialization failures as IOException. Since this checkpoint was already added to inspectedCheckpoints, those failures are silently discarded and a later checkpoint can make the test pass. Could we suppress only the missing/cleaned-up checkpoint case and rethrow other metadata read failures?
| JobID jobID, MiniCluster miniCluster) throws Exception { | ||
| final Set<Long> inspectedCheckpoints = new HashSet<>(); | ||
| final AtomicReference<String> restorePath = new AtomicReference<>(); | ||
| CommonTestUtils.waitUntilCondition( |
There was a problem hiding this comment.
Could we give this lookup an explicit deadline and include inspectedCheckpoints in the timeout failure? The one-argument waitUntilCondition loops without a timeout, while failIfJobStoppedCheckpointing only fires after the job becomes terminal. If the job remains running and completed checkpoints never contain the slow mapper's channel state, this test can hang until the outer build timeout, which keeps the original diagnosability problem.
The test asserted that the checkpoint it restores from carries in-flight input
channel state for the slow mapper, but selected that checkpoint by waiting for
the second completed checkpoint that persisted any in-flight data anywhere in
the job. Which subtask's buffers land in a given checkpoint depends on where the
barriers are when it is triggered, so the count was a calibration rather than a
condition: instrumenting the scan shows checkpoint 1 persists ~295 KB of
in-flight data while carrying no input channel state for the mapper at all, and
only checkpoint 2 has it on this hardware.
Walk the completed checkpoint history instead and restore from the first
checkpoint whose metadata really contains file-merged input channel state for
the slow mapper. The lookup reports which checkpoints it inspected if the job
terminates first, and skips checkpoints cleaned up while being read. The
SegmentFileStateHandle and state size checks stay hard assertions on the
selected checkpoint, so a file merging regression still fails loudly instead of
timing out.
What is the purpose of the change
Reduce flakiness around
FileMergingChannelStateITCaseBrief change log
Verifying this change
The test was executed 20 times to ensure the flakiness was over.
This change is a trivial rework / code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Opus 5 (1M context).