You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2021/11/29 10:27:00 UTC
[GitHub] [flink] XComp commented on a change in pull request #17912: [FLINK-24848][rest] Return 404 for unknown operation
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org