[FLINK-7709] Add CheckpointStatisticDetailsHandler for new REST endpoint #4763

tillrohrmann · 2017-10-02T17:55:44Z

What is the purpose of the change

Adds the new CheckpointStatisticDetailsHandler for the new REST server endpoint.

Moreover, this PR disables the FAIL_ON_MISSING_CREATOR_PROPERTIES property for the RestMapperUtils.getStrictObjectMapper because that is something the individuals beans can do on their own (e.g. by checking with Preconditions.checkNotNull).

R @zentol because of the changes to the ObjectMapper setup.

Verifying this change

This change is already covered by existing tests, such as CheckpointingStatisticsTest.

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)

Documentation

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

tillrohrmann · 2017-10-04T13:05:31Z

Rebasing onto the latest master.

zentol · 2017-10-04T14:14:45Z

Reviewed 24 of 31 files at r1.
Review status: 21 of 26 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/checkpoints/CheckpointStatsCache.java, line 44 at r1 (raw file):

	private final Cache<Long, AbstractCheckpointStats> cache;

	public CheckpointStatsCache(int maxNumEntries) {

The CheckpointStatsCache should be moved out of the legacy namespace.

flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/checkpoints/CheckpointStatistics.java, line 233 at r1 (raw file):

				}
			} else {
				checkpointStatisticsPerTask = Collections.emptyMap();

This method is near identical to CheckpointingStatistics#generate..., except this one line.

Comments from Reviewable

tillrohrmann · 2017-10-05T09:53:59Z

@zentol I'm not entirely sure whether reviewable makes things better or worse wrt reviewing. At least I can no longer respond to individual comments directly from github.

You're right with both comments. I'll remove the CheckpointingStatistics#generate method. Moreover, I'll move the CheckpointStatsCache out of the legacy package.

zentol · 2017-10-05T11:19:55Z

@tillrohrmann I wanted to try it out, primarily since i can mark individual files as reviewed. For the remaining files I will once again write the comments on github.

tillrohrmann · 2017-10-05T11:36:16Z

Alright @zentol. I guess it would work if I signed up for reviewable.

tillrohrmann · 2017-10-06T10:35:50Z

Quick question @zentol. You've reviewed and approved #4772 which is based on this one. Do you have more comments for this PR?

zentol · 2017-10-09T08:40:41Z

I still have to look at the JSON generation in this PR.

zentol · 2017-10-09T09:00:01Z

...e/src/main/java/org/apache/flink/runtime/rest/messages/checkpoints/CheckpointStatistics.java

+			@JsonProperty(FIELD_NAME_NUM_SUBTASKS) int numSubtasks,
+			@JsonProperty(FIELD_NAME_NUM_ACK_SUBTASKS) int numAckSubtasks,
+			@JsonDeserialize(keyUsing = JobVertexIDDeserializer.class) @JsonProperty(FIELD_NAME_TASKS) @Nullable Map<JobVertexID, TaskCheckpointStatistics> checkpointStatisticsPerTask) {
+		this.id = id;


missing checkNotNull check for all fields but checkpoitnStatisticsPerTask.

Good catch. Will add the checks.

Hmm I think all not checked parameters are actually primitives and, thus, don't need to be checked for null.

true, all objects are checked.

zentol · 2017-10-09T09:00:48Z

...e/src/main/java/org/apache/flink/runtime/rest/messages/checkpoints/CheckpointStatistics.java

+@JsonSubTypes({
+	@JsonSubTypes.Type(value = CheckpointStatistics.CompletedCheckpointStatistics.class, name = "completed"),
+	@JsonSubTypes.Type(value = CheckpointStatistics.FailedCheckpointStatistics.class, name = "failed")})
+@JsonInclude(JsonInclude.Include.NON_NULL)


We could annotate the particular field instead of the class.

I will remove the Include.NON_NULL directive and re-add the FAIL_ON_MISSING_CREATOR_PROPERTY to the ObjectMapper.

zentol · 2017-10-09T09:09:46Z

...e/src/main/java/org/apache/flink/runtime/rest/messages/checkpoints/CheckpointStatistics.java

+				throw new IllegalArgumentException("Given checkpoint stats object of type " + checkpointStats.getClass().getName() + " cannot be converted.");
+			}
+		} else {
+			return null;


pretty sure this would lead to a NullPointerException, since neither the CheckpointStatshandler, not AbstractExceutionGraphhandler, nor the AbstractRestHandler handle the case of the returned value being null.

I think it should actually work and output a serialized null value. However, this case distinction is not necessary and should be better pulled out of this method.

zentol · 2017-10-09T09:10:04Z

...e/src/main/java/org/apache/flink/runtime/rest/messages/checkpoints/CheckpointStatistics.java

+				numAckSubtasks,
+				checkpointingStatisticsPerTask);
+
+			this.externalPath = externalPath;


missing null checks

zentol · 2017-10-09T09:10:10Z

...e/src/main/java/org/apache/flink/runtime/rest/messages/checkpoints/CheckpointStatistics.java

+				numAckSubtasks,
+				checkpointingStatisticsPerTask);
+
+			this.failureTimestamp = failureTimestamp;


missing null checks

Should be a primitive.

zentol · 2017-10-09T09:11:52Z

flink-runtime/src/main/java/org/apache/flink/runtime/rest/util/RestMapperUtils.java

@@ -33,8 +33,7 @@
 		objectMapper.enable(
 			DeserializationFeature.FAIL_ON_IGNORED_PROPERTIES,
 			DeserializationFeature.FAIL_ON_NULL_FOR_PRIMITIVES,
-			DeserializationFeature.FAIL_ON_READING_DUP_TREE_KEY,
-			DeserializationFeature.FAIL_ON_MISSING_CREATOR_PROPERTIES);


Removing this also means we have to add explicit null checks to all existing Request-/ResponseBody classes.

Do note that we must be on the look-out for requests that use primitive fields, as jackson will default them to 0 if they are missing, which will cause misleading error messages.

This is a valid objection which I share. I'll remove this change and set the map of TaskCheckpointStatistics to an empty map in case that we want to leave the details out.

tillrohrmann · 2017-10-10T08:44:49Z

Thanks for the review @zentol. I've addressed your comments: Reverted change to RestMapperUtils, removed null branch from CheckpointStatistics#generateCheckpointStatistics; Made the CheckpointStatistics#checkpointStatisticsPerTask non nullable. If you have no further objections, then I would like to merge this PR once Travis gives green light.

…ointingStatistics#generateCheckpointStatistics method

…ull check out of CheckpointStatistics#generateCheckpointStatistics; Make CheckpointStatistics#checkpointStatisticcsPerTask non nullable; Add fail on missing creator property

tillrohrmann · 2017-10-10T13:18:16Z

Rebased onto the latest master

zentol · 2017-10-10T13:32:22Z

no objections, feel free to merge this.

Disable failing when not all creator properties are known Move CheckpointStatsCache out of legacy package; Remove unused CheckpointingStatistics#generateCheckpointStatistics method Remove JsonInclude.Include.NON_NULL from CheckpointStatistics; Pull null check out of CheckpointStatistics#generateCheckpointStatistics; Make CheckpointStatistics#checkpointStatisticcsPerTask non nullable; Add fail on missing creator property This closes apache#4763.

tillrohrmann force-pushed the portCheckpointStatsDetailsHandler branch from 4d69441 to d152cf2 Compare October 4, 2017 13:05

tillrohrmann force-pushed the portCheckpointStatsDetailsHandler branch from 44dfe88 to 41434f7 Compare October 5, 2017 12:50

zentol requested changes Oct 9, 2017

View reviewed changes

tillrohrmann added 4 commits October 10, 2017 15:18

[FLINK-7709] Add CheckpointStatisticDetailsHandler for new REST endpoint

32a56e0

Disable failing when not all creator properties are known

8032d89

Move CheckpointStatsCache out of legacy package; Remove unused Checkp…

4fb5dff

…ointingStatistics#generateCheckpointStatistics method

Remove JsonInclude.Include.NON_NULL from CheckpointStatistics; Pull n…

147a5b0

…ull check out of CheckpointStatistics#generateCheckpointStatistics; Make CheckpointStatistics#checkpointStatisticcsPerTask non nullable; Add fail on missing creator property

tillrohrmann force-pushed the portCheckpointStatsDetailsHandler branch from a15f452 to 147a5b0 Compare October 10, 2017 13:18

tillrohrmann closed this Oct 10, 2017

tillrohrmann deleted the portCheckpointStatsDetailsHandler branch October 10, 2017 15:36

rmetzger added component=Runtime/Coordination component=Runtime/WebFrontend component=Runtime/REST labels Mar 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-7709] Add CheckpointStatisticDetailsHandler for new REST endpoint #4763

[FLINK-7709] Add CheckpointStatisticDetailsHandler for new REST endpoint #4763

tillrohrmann commented Oct 2, 2017

tillrohrmann commented Oct 4, 2017

zentol commented Oct 4, 2017

tillrohrmann commented Oct 5, 2017

zentol commented Oct 5, 2017

tillrohrmann commented Oct 5, 2017

tillrohrmann commented Oct 6, 2017

zentol commented Oct 9, 2017

zentol Oct 9, 2017

tillrohrmann Oct 9, 2017

tillrohrmann Oct 9, 2017

zentol Oct 9, 2017

zentol Oct 9, 2017

tillrohrmann Oct 10, 2017

zentol Oct 9, 2017

tillrohrmann Oct 9, 2017 •

edited

zentol Oct 9, 2017

zentol Oct 9, 2017

tillrohrmann Oct 9, 2017

zentol Oct 9, 2017

zentol Oct 9, 2017

tillrohrmann Oct 9, 2017

tillrohrmann commented Oct 10, 2017

tillrohrmann commented Oct 10, 2017

zentol commented Oct 10, 2017

[FLINK-7709] Add CheckpointStatisticDetailsHandler for new REST endpoint #4763

[FLINK-7709] Add CheckpointStatisticDetailsHandler for new REST endpoint #4763

Conversation

tillrohrmann commented Oct 2, 2017

What is the purpose of the change

Verifying this change

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

Documentation

tillrohrmann commented Oct 4, 2017

zentol commented Oct 4, 2017

tillrohrmann commented Oct 5, 2017

zentol commented Oct 5, 2017

tillrohrmann commented Oct 5, 2017

tillrohrmann commented Oct 6, 2017

zentol commented Oct 9, 2017

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

tillrohrmann Oct 9, 2017 • edited

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

tillrohrmann commented Oct 10, 2017

tillrohrmann commented Oct 10, 2017

zentol commented Oct 10, 2017

tillrohrmann Oct 9, 2017 •

edited