Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FLINK-35157][runtime] Sources with watermark alignment get stuck once some subtasks finish #24757

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

elon-X
Copy link
Contributor

@elon-X elon-X commented May 6, 2024

What is the purpose of the change

Sources with watermark alignment get stuck once some subtasks finish, this PR solves this problem.

Brief change log

while some subtasks have been finished, the SourceOperator send Long.MAX_VALUE to SourceCoordinator, and SourceCoordinator checks whether subtasks have been finished before sending the event.

Verifying this change

This change added tests and can be verified as follows:

  • org.apache.flink.streaming.api.operators.SourceOperatorAlignmentTest::testWatermarkAlignmentWhileSubtaskFinished()
  • org.apache.flink.runtime.source.coordinator.SourceCoordinatorAlignmentTest::testWatermarkAlignmentWhileSubtaskFinished()

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API, i.e., is any changed class annotated with @Public(Evolving): no
  • The serializers: no
  • The runtime per-record code paths (performance sensitive): no
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
  • The S3 file system connector: no

Documentation

  • Does this pull request introduce a new feature? no

@flinkbot
Copy link
Collaborator

flinkbot commented May 6, 2024

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@elon-X
Copy link
Contributor Author

elon-X commented May 7, 2024

hi, @1996fanrui would you mind reviewing this for me when you have a moment? Thank you very much!

@1996fanrui 1996fanrui self-requested a review May 10, 2024 08:10
@1996fanrui 1996fanrui self-assigned this May 10, 2024
Copy link
Member

@1996fanrui 1996fanrui left a comment

Choose a reason for hiding this comment

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

Hi @elon-X , I'm wondering if "NoMoreSplitsEvent" would be appropriate to check if a subtask is finished.

The Split can be discovered dynamically. IIRC, kafka source subtask won't be FINISHED even if it doesn't have split for Flink Streaming Job. (This subtask may be assigned split after adding new kafka partition)

I prefer Gyula proposed solution: The solution could be to send out a max watermark event once the sources finish or to exclude them from the source coordinator.

We need to find a proper code place(source is definitely finished) to send MaxTimestamp.

I guess

may be a suitable place.

@@ -0,0 +1,52 @@
package org.apache.flink.test.streaming.api.datastream;
Copy link
Member

Choose a reason for hiding this comment

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

Apache license is missed here.

import static org.junit.Assert.assertThat;

/** This ITCase class tests the behavior of task execution with watermark alignment. */
public class SubTaskFinishedWithWatermarkAlignmentITCase {
Copy link
Member

Choose a reason for hiding this comment

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

nit:

How about renaming it to WatermarkAlignmentITCase?

It means all ITCases related to WatermarkAlignment can be written here. Currently, we only have one test method : testTaskFinishedWithWatermarkAlignmentExecution.

final List<Long> result = stream.executeAndCollect(101);

// Assert that the collected result contains all numbers from 0 to 100 in any order
assertThat(result, containsInAnyOrder(LongStream.rangeClosed(0, 100).boxed().toArray()));
Copy link
Member

Choose a reason for hiding this comment

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

All new test should use junit5 and assertj.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants