[FLINK-14264][StateBackend][Rest] Expose state backend in checkpoint rest api #10344

klion26 · 2019-11-28T06:48:16Z

What is the purpose of the change

Expose stateBackend in jobs/:jobId/checkpoints/config rest api

Verifying this change

This change added tests and can be verified as follows:

CheckpointConfigInfoTest#testJsonMarshalling

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, Yarn/Mesos, ZooKeeper: (no)
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)

klion26 · 2019-11-28T06:51:26Z

cc @aljoscha
another question: is there a way to automatically update the rest_api.html or do I need to update the rest_api.html manually? I followed the README in docs, seems does not generate the config.
and if there is not automatically generate script currently, do we need to add one?

flinkbot · 2019-11-28T06:51:47Z

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 fbd6e89 (Thu Nov 28 06:51:46 UTC 2019)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!
This pull request references an unassigned Jira ticket. According to the code contribution guide, tickets need to be assigned before starting with the implementation work.

_{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.

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

klion26 · 2019-11-28T06:52:31Z

...main/java/org/apache/flink/runtime/rest/handler/job/checkpoints/CheckpointConfigHandler.java

@@ -100,13 +100,15 @@ private static CheckpointConfigInfo createCheckpointConfigInfo(AccessExecutionGr
 					retentionPolicy != CheckpointRetentionPolicy.NEVER_RETAIN_AFTER_TERMINATION,
 					retentionPolicy != CheckpointRetentionPolicy.RETAIN_ON_CANCELLATION);

+			String stateName = checkpointCoordinatorConfiguration.getStateBackendName() == null ? "TEMPORARY_UNKNOWN" : checkpointCoordinatorConfiguration.getStateBackendName();


If we query the rest before the job is running, maybe the checkpointCoordinatorConfiguration.getStateBackendName() will return null (please see the above comments), so transfer to TEMPORARY_UNKNOWN here.

how would you query for a job before it is submitted? You'd end up with 404 in all cases.

@zentol Thanks for the clarification. Before I submit the PR, I tried a few times to query the rest before job submitted, but the job submitting duration holds too small, all end up with a running job.
Maybe we should add a checkNotNull here for stateBackendName, what do you think.

klion26 · 2019-11-28T06:55:57Z

...rc/main/java/org/apache/flink/runtime/jobgraph/tasks/CheckpointCoordinatorConfiguration.java

+		return stateBackendName;
+	}
+
+	public void setStateBackendName(@Nonnull String newStateBackend) {


Add a setter function here instead of passed in from constructor, because StreamGraph#getStateBackend() may return null in StreamingJobGraphGenerator#configureCheckpointing

flinkbot · 2019-11-28T07:15:30Z

CI report:

fbd6e89 : SUCCESS Build
1793d65 : SUCCESS Build
7ce0490 : SUCCESS Build
5133d0c : SUCCESS Build
7268c52 : CANCELED Build
1e14444 : FAILURE Build

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build

aljoscha · 2019-11-28T10:40:04Z

@klion26 It's a bit harder to find but there is this: https://github.com/apache/flink/blob/e1ca4c46ea7e2940b0200b635bc49766d594ffb0/flink-docs/README.md

klion26 · 2019-11-28T13:43:38Z

@aljoscha thanks for the reply, have generated the rest document.

zentol

I don't think this is a viable approach; unfortunately I couldn't quickly come up with a proper way of doing it.

zentol · 2019-11-28T16:27:34Z

docs/_includes/generated/rest_v1_dispatcher.html

@@ -2526,7 +2529,7 @@
      "type" : "array",
      "items" : {
        "type" : "object",
-        "id" : "urn:jsonschema:org:apache:flink:runtime:rest:messages:SubtaskExecutionAttemptDetailsInfo",
+        "id" : "urn:jsonschema:org:apache:flink:runtime:rest:messages:job:SubtaskExecutionAttemptDetailsInfo",


it appears the api docs were a bit outdated; can you move this change and below ones into a separate commit?

ok, will update it

zentol · 2019-11-28T16:30:12Z

...rc/main/java/org/apache/flink/runtime/jobgraph/tasks/CheckpointCoordinatorConfiguration.java

@@ -36,6 +38,8 @@

 	private static final long serialVersionUID = 2L;

+	private String stateBackendName;


this doesn't seem like the right place to put it; we are polluting a runtime configuration class with information that is only relevant to the REST API. This class should only contain things that are relevant for the CheckpointCoordinator; that's this class purpose after all.

I think it's the name here that makes people confused. the stateBackendName here is checkpoint backend name, such as MemoryStateBackend, FsStateBackend or RocksDBStateBackend, and I think they'are relevant with CheckpointCoordinator, we'll also pass the backend into CheckpointCoordinator to init the checkpoint directory.
Maybe we could update the name here, what do you think?

What functional purpose would this field have for the coordinator? It already has the actual backend.

Ahh, got it.
Maybe we should get the stateBackendName from AccessExecutionGraph directly(add a new function getStateBackend in AccessExecutionGraph, pass the stateBackendName into ArchivedExecutionGraph when init a new ArchivedExecutionGraph), what do you think?

That's a better option.

I would suggest adding CheckpointCoordinator#getStateBackendName() that is called in ArchivedExecutionGraph#from(). Your plan should cover the rest.

zentol · 2019-11-28T16:31:52Z

...main/java/org/apache/flink/runtime/rest/handler/job/checkpoints/CheckpointConfigHandler.java

@@ -100,13 +100,15 @@ private static CheckpointConfigInfo createCheckpointConfigInfo(AccessExecutionGr
 					retentionPolicy != CheckpointRetentionPolicy.NEVER_RETAIN_AFTER_TERMINATION,
 					retentionPolicy != CheckpointRetentionPolicy.RETAIN_ON_CANCELLATION);

+			String stateName = checkpointCoordinatorConfiguration.getStateBackendName() == null ? "TEMPORARY_UNKNOWN" : checkpointCoordinatorConfiguration.getStateBackendName();


how would you query for a job before it is submitted? You'd end up with 404 in all cases.

klion26 · 2019-12-02T03:18:23Z

@zentol thanks for your review.
First, I want to set the checkpoint backend name in StreamingJobGraphGenerator, so we do not need to update the checkpoint backend name after, but we may get a null when calling streamGraph.getStateBackend(), so I changed to udpate the checkpoint backend name as currently in ExecutionGraphBuilder which can get the truely checkpoint backend name, this checkpoint backend will also be passed into CheckpointCoordinator to create the checkpoint storage.

klion26 · 2019-12-05T09:32:51Z

Currently, I set the default value of "stateBackend" to "disabled", because when I run job(disabled periodic checkpoint) on yarn, I can still get the checkpoint config from rest.

I find that in CheckpointConfigHandler we use checkpointConfig == null to judge whether enable or disable checkpoint, and the doc of AccessExecutionGraph#getCheckpointCoordinatorConfiguration says that Returns the {@link CheckpointCoordinatorConfiguration} or <code>null</code> if checkpointing is disabled., but after disabling checkpoint with rocksdbbackend, I still can get the config from rest api.

I'll find out why this happens and come back.

klion26 · 2019-12-05T11:20:21Z

Currently, we use jobGraph.getCheckpointingSettings() == null to test whether enabled Checkpoint, but when generating JobGraph in client-side, we'll always set checkpointSettings, so we'll always create CheckpointCoordinator whether or not enable checkpoint.

I think we can create another issue to use jobGraph.isCheckpointingEnabled() instead of jobGraph.getCheckpointingSettings() == null to test whethe enable checkpoint or not, so we can init stateBackendName with null here, because we'll always fail if checkpoint is disabled, and wil never call getStateName. what do you about this? @zentol

zentol · 2019-12-05T15:03:01Z

So I asked around and the reason why the CC is created even if checkpointing is disabled is that you still need it to support savepoints.
In other words, the existing code only differentiates between batch/streaming jobs; but all streaming jobs are handled the same. Let's not touch this code.

because we'll always fail if checkpoint is disabled

Which part would be failing right now?

klion26 · 2019-12-06T02:08:04Z

Which part would be failing right now?

Currently, not part will fail right now. I said that it will always fail is that if we do not create CC after disabled checkpoint, then the rest for checkpoint config will always fail

Before I find that the rest will throw exception with reason RestHandlerException( "Checkpointing is not enabled for this job (" + executionGraph.getJobID() + ").", HttpResponseStatus.NOT_FOUND), so I thought the rest will always "fail" if checkpoint has been disabled, but it did not. even checkpoint has been disabled, we can get a result from rest with the interval of Long.MAX, so I set the default value of stateBackendName to disabled, but seems the default value will never be used :(, if we create CC, then rest will always return the truly backendName, if we do not create cc, then rest will fail, and will not read the backendName. So maybe we can still set the default value to null for stateBackendName here. what do you think? @zentol

Apart from the default value of stateBackendName the PR is ready for review now.

zentol · 2019-12-06T09:13:44Z

I'm fine with setting it to null by default; it's a bit odd that the CC can be null in the ArchivedEG but not the stateBackendName.

klion26 · 2019-12-06T09:53:57Z

@zentol thanks for the replay, updated it.

rmetzger added the review=description? label Nov 28, 2019

klion26 commented Nov 28, 2019

View reviewed changes

rmetzger added component=Runtime/StateBackends component=Runtime/REST labels Nov 28, 2019

zentol self-assigned this Nov 28, 2019

zentol requested changes Nov 28, 2019

View reviewed changes

klion26 force-pushed the 14264 branch from 1793d65 to 7ce0490 Compare December 2, 2019 03:23

[FLINK-14264][rest] Expose state backend name

1e14444

zentol force-pushed the 14264 branch from 7268c52 to 1e14444 Compare December 6, 2019 13:36

zentol merged commit 3005f04 into apache:master Dec 8, 2019

klion26 deleted the 14264 branch January 3, 2024 12:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-14264][StateBackend][Rest] Expose state backend in checkpoint rest api #10344

[FLINK-14264][StateBackend][Rest] Expose state backend in checkpoint rest api #10344

klion26 commented Nov 28, 2019

klion26 commented Nov 28, 2019

flinkbot commented Nov 28, 2019

klion26 Nov 28, 2019

zentol Nov 28, 2019

klion26 Dec 2, 2019

klion26 Nov 28, 2019

flinkbot commented Nov 28, 2019 •

edited

aljoscha commented Nov 28, 2019

klion26 commented Nov 28, 2019

zentol left a comment

zentol Nov 28, 2019

klion26 Dec 2, 2019

zentol Nov 28, 2019

klion26 Dec 2, 2019

zentol Dec 2, 2019

klion26 Dec 2, 2019

zentol Dec 4, 2019

zentol Nov 28, 2019

klion26 commented Dec 2, 2019

klion26 commented Dec 5, 2019

klion26 commented Dec 5, 2019

zentol commented Dec 5, 2019 •

edited

klion26 commented Dec 6, 2019 •

edited

zentol commented Dec 6, 2019

klion26 commented Dec 6, 2019

		@@ -36,6 +38,8 @@

		private static final long serialVersionUID = 2L;

		private String stateBackendName;

[FLINK-14264][StateBackend][Rest] Expose state backend in checkpoint rest api #10344

[FLINK-14264][StateBackend][Rest] Expose state backend in checkpoint rest api #10344

Conversation

klion26 commented Nov 28, 2019

What is the purpose of the change

Verifying this change

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

Documentation

klion26 commented Nov 28, 2019

flinkbot commented Nov 28, 2019

Automated Checks

Review Progress

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flinkbot commented Nov 28, 2019 • edited

CI report:

aljoscha commented Nov 28, 2019

klion26 commented Nov 28, 2019

zentol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klion26 commented Dec 2, 2019

klion26 commented Dec 5, 2019

klion26 commented Dec 5, 2019

zentol commented Dec 5, 2019 • edited

klion26 commented Dec 6, 2019 • edited

zentol commented Dec 6, 2019

klion26 commented Dec 6, 2019

flinkbot commented Nov 28, 2019 •

edited

zentol commented Dec 5, 2019 •

edited

klion26 commented Dec 6, 2019 •

edited