leletan commented on code in PR #20852: URL: https://github.com/apache/flink/pull/20852#discussion_r992624496
########## 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: Good observation, @zentol. This is where I was struggling as well. Here are some context why I was slightly leaning towards trigger: 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`) 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. 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. Meanwhile I have to admit 1. each manual trigger only corresponds to at-most 1 checkpoint in the `DispatcherOperationCaches` thus it is making it confusing. 2. this is very similar to savepoint logic, which has no "trigger" for its API or class names. That's why I was struggling and slightly leaning towards having "trigger" in the API and class names. However if you still feel strongly the naming should be without "trigger", I have no problem removing them. I may missed something and feel free to let me know. -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
