SteveStevenpoor commented on code in PR #28689:
URL: https://github.com/apache/flink/pull/28689#discussion_r3546069662
##########
flink-runtime/src/main/java/org/apache/flink/streaming/api/operators/SourceOperator.java:
##########
@@ -848,12 +848,16 @@ private void checkSplitWatermarkAlignment() {
Collection<String> splitsToResume = new ArrayList<>();
sampledSplitWatermarks.forEach(
(splitId, splitWatermarks) -> {
- if (currentlyIdleSplits.contains(splitId)) {
- return;
- }
if (splitWatermarks.getOldestSample() >
currentMaxDesiredWatermark) {
Review Comment:
I’m still concerned about the case where we mark an idle split as paused:
`markPaused()` marks it as not idle, but the split remains in
`currentlyIdleSplits`.
So we can get the following sequence:
1. A split is paused.
2. During the pause process, the split is marked idle.
3. `reportPausedOrResumed()` is called and marks the split as not idle.
4. However, the split still remains in `currentlyIdleSplits`.
As a result, before the next alignment, the split can be non-idle according
to the metric group, but still present in both `currentlyIdleSplits` and
`currentlyPausedSplits`.
I’m not sure whether this affects timers or checks like:
```java
idle == currentlyIdleSplits.contains(splitId)
```
but this inconsistency still looks risky to me.
##########
flink-runtime/src/test/java/org/apache/flink/streaming/api/operators/SourceOperatorSplitWatermarkAlignmentTest.java:
##########
@@ -569,6 +569,58 @@ void testAlignmentCheckIsDeferredForIdleSplits() throws
Exception {
0L,
operator.getSplitMetricGroup(split0.splitId()).getAccumulatedPausedTime());
}
+ @Test
+ void testPausedIdleSplitsCanBeResumedByAlignmentCheck() throws Exception {
+ final long idleTimeout = 100;
+ final MockSourceReader sourceReader =
+ new MockSourceReader(WaitingForSplits.DO_NOT_WAIT_FOR_SPLITS,
true, true);
+ final TestProcessingTimeService processingTimeService = new
TestProcessingTimeService();
+ final SourceOperator<Integer, MockSourceSplit> operator =
+ createAndOpenSourceOperatorWithIdleness(
+ sourceReader, processingTimeService, idleTimeout);
+
+ final MockSourceSplit split0 = new MockSourceSplit(0, 0, 10);
+ final int allowedWatermark4 = 4;
+ final int allowedWatermark7 = 7;
+ split0.addRecord(4);
+ split0.addRecord(5);
+ split0.addRecord(6);
+ split0.addRecord(7);
+ split0.addRecord(8);
+ operator.handleOperatorEvent(
+ new AddSplitEvent<>(Arrays.asList(split0), new
MockSourceSplitSerializer()));
+ final CollectingDataOutput<Integer> actualOutput = new
CollectingDataOutput<>();
+
+ // Emit enough records to fill the sampler buffer
+ for (int i = 0; i < WATERMARK_ALIGNMENT_BUFFER_SIZE.defaultValue();
i++) {
+ operator.emitNext(actualOutput);
+ processingTimeService.advance(idleTimeout - 1);
+ }
+ sampleAllWatermarks(processingTimeService);
+ assertOutput(actualOutput, Arrays.asList(4, 5, 6));
+
+ // Alignment check fires and pauses the split
+ operator.handleOperatorEvent(new
WatermarkAlignmentEvent(allowedWatermark4));
+
assertThat(operator.getSplitMetricGroup(split0.splitId()).isPaused()).isTrue();
+ assertThat(sourceReader.getPausedSplits()).containsExactly("0");
+ assertOutput(actualOutput, Arrays.asList(4, 5, 6));
+
+ // Normally idlenessTimer can't elapse while the split is paused
+ // So calling it manually to simulate a race condition
+ operator.updateCurrentSplitIdle(split0.splitId(), true);
Review Comment:
Here you mark the split as idle only after the full pauseOrResumeSplits
process has finished. To simulate the issue being addressed, you can use
something like this:
```java
private static class RaceInjectingMockSourceReader extends
MockSourceReader {
private Runnable afterNextPause = () -> {};
RaceInjectingMockSourceReader(
WaitingForSplits waitingForSplitsBehaviour,
boolean markIdleOnNoSplits,
boolean usePerSplitOutputs) {
super(waitingForSplitsBehaviour, markIdleOnNoSplits,
usePerSplitOutputs);
}
void runAfterNextPause(Runnable afterNextPause) {
this.afterNextPause = afterNextPause;
}
@Override
public void pauseOrResumeSplits(
Collection<String> splitsToPause, Collection<String>
splitsToResume) {
super.pauseOrResumeSplits(splitsToPause, splitsToResume);
if (!splitsToPause.isEmpty()) {
Runnable callback = afterNextPause;
afterNextPause = () -> {};
callback.run();
}
}
}
```
Then in the test, you can use:
```java
sourceReader.runAfterNextPause(
() -> operator.updateCurrentSplitIdle(split0.splitId(),
true));
```
This will mark the split idle right after the actual pause.
```java
private void pauseOrResumeSplits(
Collection<String> splitsToPause, Collection<String>
splitsToResume) {
try {
LOG.info(
"pauseOrResumeSplits
[splitsToPause={}][splitsToResume={}][idleSplits={}]"
+
"[currentMaxDesiredWatermark={}][latestWatermark={}][oldestWatermark={}]",
splitsToPause,
splitsToResume,
currentlyIdleSplits,
currentMaxDesiredWatermark,
sampledLatestWatermark.getLatest(),
sampledLatestWatermark.getOldestSample());
sourceReader.pauseOrResumeSplits(splitsToPause, splitsToResume);
// the split is marked idle
eventTimeLogic.pauseOrResumeSplits(splitsToPause,
splitsToResume);
reportPausedOrResumed(splitsToPause, splitsToResume);
} catch (UnsupportedOperationException e) {
if (!allowUnalignedSourceSplits) {
throw e;
}
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
