XComp commented on a change in pull request #17912:
URL: https://github.com/apache/flink/pull/17912#discussion_r758216170
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/job/savepoints/SavepointHandlers.java
##########
@@ -305,4 +335,15 @@ protected SavepointInfo operationResultResponse(String
operationResult) {
return new SavepointInfo(operationResult, null);
}
}
+
+ private static CompletionException getInternalServerError(
Review comment:
nit: `createInternalServerError`
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/job/savepoints/SavepointHandlers.java
##########
@@ -267,29 +278,48 @@ public SavepointStatusHandler(
final AsynchronousJobOperationKey key = getOperationKey(request);
return gateway.getTriggeredSavepointStatus(key)
- .thenApply(
- (operationResult) -> {
- switch (operationResult.getStatus()) {
- case SUCCESS:
- return
AsynchronousOperationResult.completed(
- operationResultResponse(
-
operationResult.getResult()));
- case FAILURE:
- return
AsynchronousOperationResult.completed(
-
exceptionalOperationResultResponse(
-
operationResult.getThrowable()));
- case IN_PROGRESS:
- return
AsynchronousOperationResult.inProgress();
- default:
- throw new IllegalStateException(
- "No handler for operation
status "
- +
operationResult.getStatus()
- + ", encountered for
key "
- + key);
+ .handle(
+ (operationResult, throwable) -> {
+ if (throwable == null) {
+ switch (operationResult.getStatus()) {
+ case SUCCESS:
+ return
AsynchronousOperationResult.completed(
+ operationResultResponse(
+
operationResult.getResult()));
+ case FAILURE:
+ return
AsynchronousOperationResult.completed(
+
exceptionalOperationResultResponse(
+
operationResult.getThrowable()));
+ case IN_PROGRESS:
+ return
AsynchronousOperationResult.inProgress();
+ default:
+ throw new IllegalStateException(
+ "No handler for operation
status "
+ +
operationResult.getStatus()
+ + ", encountered
for key "
+ + key);
+ }
+ } else {
+ maybeHandleUnknownOperation(throwable,
key);
Review comment:
I don't like the method name. It does not indicate that an exception
might be thrown here as well. As an alternative, we could generate an
`Optional` of the `CompletionException` that is returned by the method and
thrown in the else branch. That would make it more obvious.
Or we remove the method completely since it's only called at this location.
But I feat that this might decrease the readability.
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/job/savepoints/SavepointHandlers.java
##########
@@ -267,29 +278,48 @@ public SavepointStatusHandler(
final AsynchronousJobOperationKey key = getOperationKey(request);
return gateway.getTriggeredSavepointStatus(key)
- .thenApply(
- (operationResult) -> {
- switch (operationResult.getStatus()) {
- case SUCCESS:
- return
AsynchronousOperationResult.completed(
- operationResultResponse(
-
operationResult.getResult()));
- case FAILURE:
- return
AsynchronousOperationResult.completed(
-
exceptionalOperationResultResponse(
-
operationResult.getThrowable()));
- case IN_PROGRESS:
- return
AsynchronousOperationResult.inProgress();
- default:
- throw new IllegalStateException(
- "No handler for operation
status "
- +
operationResult.getStatus()
- + ", encountered for
key "
- + key);
+ .handle(
+ (operationResult, throwable) -> {
+ if (throwable == null) {
+ switch (operationResult.getStatus()) {
+ case SUCCESS:
+ return
AsynchronousOperationResult.completed(
+ operationResultResponse(
+
operationResult.getResult()));
+ case FAILURE:
+ return
AsynchronousOperationResult.completed(
+
exceptionalOperationResultResponse(
+
operationResult.getThrowable()));
+ case IN_PROGRESS:
+ return
AsynchronousOperationResult.inProgress();
+ default:
+ throw new IllegalStateException(
+ "No handler for operation
status "
+ +
operationResult.getStatus()
+ + ", encountered
for key "
+ + key);
+ }
+ } else {
+ maybeHandleUnknownOperation(throwable,
key);
Review comment:
> Or we remove the method completely since it's only called at this
location. But I feat that this might decrease the readability.
Thinking about it once more: Should we handle UnknownOperations in
`SavepointHandlerBase#triggerOperation` 🤔
--
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]
