Skip to content

[Dataflow Streaming] Mark worker as unhealthy in presence of stuck commits - #39666

Merged
tvalentyn merged 3 commits into
apache:masterfrom
arunpandianp:unhealthyStuckCommit
Aug 11, 2026
Merged

[Dataflow Streaming] Mark worker as unhealthy in presence of stuck commits#39666
tvalentyn merged 3 commits into
apache:masterfrom
arunpandianp:unhealthyStuckCommit

Conversation

@arunpandianp

Copy link
Copy Markdown
Contributor

Getting rid of the commit invalidation logic simplifies work lifecycle. When worker is marked as unhealthy backend will recreate the worker.

Ran a job with a delay added in commit path and verified that the worker got recreated.

…mmits

Getting rid of the commit invalidation logic simplifies work lifecycle.
When worker is marked as unhealthy backend will restart the worker.
@arunpandianp

Copy link
Copy Markdown
Contributor Author

R: @scwhittle

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Assigning reviewers:

R: @tvalentyn added as fallback since no labels match configuration

Note: If you would like to opt out of this review, comment assign to next reviewer.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

int stuckCommitDurationMillis =
options.isEnableStreamingEngine() ? Math.max(options.getStuckCommitDurationMillis(), 0) : 0;
if (stuckCommitDurationMillis <= 0) {
return true;

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.

nit: how about nesting the loop under an if stuckCommitsDuration > 0, instead of the early return true. Then we can add other things that return false if it's unhealthy and not accidentally skip them.

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.

Done.

assertThat(invalidatedCommits).containsEntry(shardedKey2, stuckWork2.id());
verify(computationStateCache).invalidate(shardedKey1.key(), shardedKey1.shardingKey());
verify(computationStateCache).invalidate(shardedKey2.key(), shardedKey2.shardingKey());
assertThat(activeWorkState.hasStuckCommits(Instant.now())).isTrue();

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.

validate it is false with timestamp closer to start procesisng timestamp

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.

done.

@tvalentyn
tvalentyn merged commit f353f12 into apache:master Aug 11, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants