[FLINK-17073][checkpointing] Backpressure checkpointing on slow cleanup #13565

rkhachatryan · 2020-10-08T17:47:09Z

What is the purpose of the change

This PR includes changes from #13040 and some refactorings of new and existing code.

Verifying this change

Changes for backpressure tested by new ZooKeeperCompletedCheckpointStoreITCase.testChekpointingPausesAndResumeWhenTooManyCheckpoints test

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/Mesos, ZooKeeper: yes
The S3 file system connector: no

Documentation

Does this pull request introduce a new feature? no
If yes, how is the feature documented? not applicable

…e for asynchronous checkpoints cleaning via the ioExecutor. This class counts the number of checkpoints to clean and reports it to CheckpointRequestDecider. Add a test for too many checkpoints to clean.

…nt classes Pass CheckpointsCleaner and postCleanup as arguments instead of storing them in Checkpoints classes as callbacks.

Pass Executor as an argument instead of storing it as a field.

…nt store

flinkbot · 2020-10-08T17:49:22Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 82386f6 (Fri May 28 07:00:20 UTC 2021)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

Details

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

flinkbot · 2020-10-08T17:56:40Z

CI report:

82386f6 Azure: FAILURE

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build
@flinkbot run azure re-run the last Azure build

etienne-chauchot · 2020-10-12T16:20:16Z

@rkhachatryan I don't see my ITest commit in this history. Does this mean that you don't want to merge this part of the code or is it just that the commit was forgotten?

pnowojski

I think @etienne-chauchot your test was squashed with the commit that was adding the feature.

flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/PendingCheckpoint.java

etienne-chauchot · 2020-10-12T16:56:26Z

I think @etienne-chauchot your test was squashed with the commit that was adding the feature.

Ok fair enough

pnowojski

LGTM, @rkhachatryan could you squash the fixup commit?

…tsCleaner Replace cleanup Runnable with a direct call to CheckpointsCleaner.

rkhachatryan · 2020-10-14T13:26:28Z

@pnowojski, I squashed the commit but the build failed.
The reason is unrelated: FLINK-19619 (both public and private repos).

pnowojski · 2020-10-14T14:27:07Z

Thanks for the contribution both @rkhachatryan and @etienne-chauchot . Merging.

tillrohrmann · 2021-01-04T15:53:18Z

flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CompletedCheckpointStore.java

 	 * @param jobStatus Job state on shut down
 	 */
-	void shutdown(JobStatus jobStatus) throws Exception;
+	void shutdown(JobStatus jobStatus, CheckpointsCleaner checkpointsCleaner, Runnable postCleanup) throws Exception;


Why do we need the postCleanup? In the current code base I couldn't find a single usage where we don't pass () -> {} @rkhachatryan and @pnowojski? If this is not needed, can we get rid of it? I think it does not make a very nice API with the no-ops.

Moreover, it would be nice if we could update the JavaDocs if we change the signature of interfaces.

postCleanup is used by checkpointsCleaner but not in case of shutting down. I agree, it should be removed (by pushing down to DefaultCompletedCheckpointStore) and javadoc added. My bad.
I've created FLINK-20847 to address this.

echauchot and others added 4 commits October 8, 2020 15:48

[FLINK-17073] [checkpointing] Introduce CheckpointsCleaner responsibl…

0ecc402

…e for asynchronous checkpoints cleaning via the ioExecutor. This class counts the number of checkpoints to clean and reports it to CheckpointRequestDecider. Add a test for too many checkpoints to clean.

[FLINK-17073][checkpointing][refactor] Remove callbacks from Checkpoi…

2455dcb

…nt classes Pass CheckpointsCleaner and postCleanup as arguments instead of storing them in Checkpoints classes as callbacks.

[hotfix][checkpointing][refactor] Remove PendingCheckpoint.executor

5d75e49

Pass Executor as an argument instead of storing it as a field.

[FLINK-17073][checkpointing][refactor] Remove callback in ZK checkpoi…

2442332

…nt store

rmetzger added the review=description? label Oct 8, 2020

rmetzger added the component=Runtime/Checkpointing label Oct 8, 2020

pnowojski reviewed Oct 12, 2020

View reviewed changes

flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/PendingCheckpoint.java Show resolved Hide resolved

pnowojski approved these changes Oct 14, 2020

View reviewed changes

[FLINK-17073][checkpointing][refactor] Remove callback from Checkpoin…

82386f6

…tsCleaner Replace cleanup Runnable with a direct call to CheckpointsCleaner.

rkhachatryan force-pushed the f17073-refactor-fixup branch from 2d4a962 to 82386f6 Compare October 14, 2020 10:18

pnowojski merged commit 318c7b6 into apache:master Oct 14, 2020

pnowojski mentioned this pull request Oct 14, 2020

[FLINK-17073] [checkpointing] checkpointing backpressure if there are too many checkpoints to clean #13040

Closed

tillrohrmann reviewed Jan 4, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[FLINK-17073][checkpointing] Backpressure checkpointing on slow cleanup #13565

[FLINK-17073][checkpointing] Backpressure checkpointing on slow cleanup #13565

Uh oh!

rkhachatryan commented Oct 8, 2020

Uh oh!

flinkbot commented Oct 8, 2020 •

edited

Loading

Uh oh!

flinkbot commented Oct 8, 2020 •

edited

Loading

Uh oh!

etienne-chauchot commented Oct 12, 2020 •

edited

Loading

Uh oh!

pnowojski left a comment

Uh oh!

Uh oh!

etienne-chauchot commented Oct 12, 2020

Uh oh!

pnowojski left a comment

Uh oh!

rkhachatryan commented Oct 14, 2020 •

edited

Loading

Uh oh!

pnowojski commented Oct 14, 2020

Uh oh!

tillrohrmann Jan 4, 2021

Uh oh!

rkhachatryan Jan 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

[FLINK-17073][checkpointing] Backpressure checkpointing on slow cleanup #13565

[FLINK-17073][checkpointing] Backpressure checkpointing on slow cleanup #13565

Uh oh!

Conversation

rkhachatryan commented Oct 8, 2020

What is the purpose of the change

Verifying this change

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

Documentation

Uh oh!

flinkbot commented Oct 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automated Checks

Review Progress

Uh oh!

flinkbot commented Oct 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

etienne-chauchot commented Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pnowojski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

etienne-chauchot commented Oct 12, 2020

Uh oh!

pnowojski left a comment

Choose a reason for hiding this comment

Uh oh!

rkhachatryan commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pnowojski commented Oct 14, 2020

Uh oh!

tillrohrmann Jan 4, 2021

Choose a reason for hiding this comment

Uh oh!

rkhachatryan Jan 4, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

flinkbot commented Oct 8, 2020 •

edited

Loading

flinkbot commented Oct 8, 2020 •

edited

Loading

etienne-chauchot commented Oct 12, 2020 •

edited

Loading

rkhachatryan commented Oct 14, 2020 •

edited

Loading