zentol commented on code in PR #20852: URL: https://github.com/apache/flink/pull/20852#discussion_r993242999
########## flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/checkpoints/CheckpointTriggerStatusHeaders.java: ########## @@ -0,0 +1,82 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.flink.runtime.rest.messages.checkpoints; + +import org.apache.flink.runtime.rest.HttpMethodWrapper; +import org.apache.flink.runtime.rest.handler.async.AsynchronousOperationStatusMessageHeaders; +import org.apache.flink.runtime.rest.messages.EmptyRequestBody; +import org.apache.flink.runtime.rest.messages.JobIDPathParameter; +import org.apache.flink.runtime.rest.messages.TriggerIdPathParameter; + +import org.apache.flink.shaded.netty4.io.netty.handler.codec.http.HttpResponseStatus; + +/** These headers define the protocol for triggering a checkpoint. */ +public class CheckpointTriggerStatusHeaders + extends AsynchronousOperationStatusMessageHeaders< + CheckpointTriggerInfo, CheckpointTriggerStatusMessageParameters> { Review Comment: > this is very similar to savepoint logic, which has no "trigger" for its API or class names. That's why I'd like avoid it. It behaves the same way but currently has a different naming scheme. > 1. The new APIs in this PR is for manual trigger of a checkpoint and it may or may not end up with a checkpoint (it could end up with a failure and return with a failure cause, before it can reach CheckpointCoordinator) That's a good observation but I'd say it's fine nonetheless to not refer to "triggers". It's just a technical limitation that we can't differentiate between failures before/after the checkpoint/savepoint was triggered that we _may_ fix at some point. I also think this distinction isn't really important. It would of course be great if the initial request could already fail early, but at the end of the day the results are the same to the user. > 2. The checkpoint trigger status is queried by trigger id and its value is read from DispatcherOperationCaches which has only 5 min cache duration by default, so it is more of a trigger status than checkpoint status. The timeout is there because the JM waits with it's shutdown until the result of these requests have been consumed, and we don't want it to sit there idling indefinitely because someone forget to consume a checkpoint result. > 3. There are already other APIs for querying checkpoint metadata and details, which should cover all the retained checkpoints for the application - don't want to confuse user with them. Interesting point. However this also applies to the savepoints API and so far no one complained. I think there's a well-defined use-cases for both checkpoint-related APIs; one gives you the full details, the other just status information about manually triggered checkpoints (which is somewhat explicit since the url has a triggerId in it). I wouldn't worry about it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
