Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,9 @@ public void markPaused() {
public void markIdle() {
maybeMarkSplitStart();
if (isPaused()) {
// If a split is marked idle, it has no records to emit.
// hence it shouldn't be considered paused anymore
markNotPaused();
LOG.warn("[{}] Split marked idle while still paused", splitId);
// This is benign: idleness takes over paused state if they race
LOG.info("[{}] Split marked idle while still paused", splitId);
}
this.idleTimePerSecond.markStart();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

idle == currentlyIdleSplits.contains(splitId)

but this inconsistency still looks risky to me.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we mark an idle split as paused: markPaused() marks it as not idle, but the split remains in currentlyIdleSplits.

Pause is skipped for idle splits.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we mark an idle split as paused: markPaused() marks it as not idle, but the split remains in currentlyIdleSplits.

Pause is skipped for idle splits.

Yeah, I mean the case when the split is marked idle during the pause process (it was not idle at decision time, so it could still be added to splitToPause).

Anyway, I agree this is probably not a blocker for this PR.

// Skipping pause for idle splits so we won't clear their idleness
if (currentlyIdleSplits.contains(splitId)) {
LOG.info("[{}] Skipping pause for idle split", splitId);
return;
}
splitsToPause.add(splitId);
} else if (currentlyPausedSplits.contains(splitId)) {
// Resuming possibly-idle splits without clearing their idleness state
// (the next record to arrive will do it naturally)
splitsToResume.add(splitId);
}
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -569,6 +569,66 @@ void testAlignmentCheckIsDeferredForIdleSplits() throws Exception {
0L, operator.getSplitMetricGroup(split0.splitId()).getAccumulatedPausedTime());
}

@Test
void testPausedIdleSplitsCanBeResumedByAlignmentCheck() throws Exception {
final long idleTimeout = 1000;
final long autoWatermarkIntervalMillis = 100;
final long marginMillis = autoWatermarkIntervalMillis / 2;
final MockSourceReader sourceReader =
new MockSourceReader(WaitingForSplits.DO_NOT_WAIT_FOR_SPLITS, true, true);
final TestProcessingTimeService processingTimeService = new TestProcessingTimeService();
processingTimeService.setCurrentTime(0);
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(5);
split0.addRecord(6);
operator.handleOperatorEvent(
new AddSplitEvent<>(
Collections.singletonList(split0), new MockSourceSplitSerializer()));
final CollectingDataOutput<Integer> actualOutput = new CollectingDataOutput<>();

operator.emitNext(actualOutput); // 5
sampleAllWatermarks(
processingTimeService); // fill the alignment ring buffer with watermark=5
final long timeElapsed =
WATERMARK_ALIGNMENT_BUFFER_SIZE.defaultValue() * updateIntervalMillis;

// Advancing in small steps (TestProcessingTimeService limitation)
processingTimeService.advance(2 * autoWatermarkIntervalMillis - timeElapsed); // t=200
processingTimeService.advance(idleTimeout); // t=1200
processingTimeService.advance(marginMillis); // t=1250

// Firing alignment check at t=1250.
// Idleness clock will be now paused 250ms past the timeout (1000),
// which means it will fire on the next idleness periodic check
operator.handleOperatorEvent(new WatermarkAlignmentEvent(allowedWatermark4));
assertThat(operator.getSplitMetricGroup(split0.splitId()).isPaused()).isTrue();
assertThat(sourceReader.getPausedSplits()).containsExactly("0");
assertOutput(actualOutput, Arrays.asList(5));

// Triggerring the next idleness periodic check
// which marks the split idle while paused.
processingTimeService.advance(autoWatermarkIntervalMillis + marginMillis); // t=1400
assertThat(operator.getSplitMetricGroup(split0.splitId()).isIdle()).isTrue();
assertThat(sourceReader.getPausedSplits()).containsExactly("0");

// Watermark advances and resumes the split
// (Though it is still considered idle - this is what this commit is about)
operator.handleOperatorEvent(new WatermarkAlignmentEvent(allowedWatermark7));
assertThat(operator.getSplitMetricGroup(split0.splitId()).isIdle()).isTrue();
assertThat(sourceReader.getPausedSplits()).isEmpty();

// The split emits a record and breaks out of idleness
operator.emitNext(actualOutput); // 6
assertThat(operator.getSplitMetricGroup(split0.splitId()).isActive()).isTrue();
assertOutput(actualOutput, Arrays.asList(5, 6));
}

private void sampleAllWatermarks(TestProcessingTimeService timeService) throws Exception {
sampleWatermarks(timeService, WATERMARK_ALIGNMENT_BUFFER_SIZE.defaultValue());
}
Expand Down