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/10/01 08:16:37 UTC
[GitHub] [flink] nicoweidner commented on a change in pull request #17390: [FLINK-24275][rest] Idempotent job cancellation
nicoweidner commented on a change in pull request #17390:
URL: https://github.com/apache/flink/pull/17390#discussion_r720015258
##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/rest/RestMatchers.java
##########
@@ -0,0 +1,83 @@
+/*
+ * 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;
+
+import org.apache.flink.runtime.rest.handler.RestHandlerException;
+
+import org.apache.flink.shaded.netty4.io.netty.handler.codec.http.HttpResponseStatus;
+
+import org.hamcrest.Description;
+import org.hamcrest.Matcher;
+import org.hamcrest.TypeSafeDiagnosingMatcher;
+
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
+
+/** Hamcrest matchers for REST handlers. */
+public class RestMatchers {
+
+ public static <T> Matcher<CompletableFuture<T>> respondsWithError(
+ HttpResponseStatus responseStatus) {
+ return new ErrorResponseStatusCodeMatcher<>(responseStatus);
+ }
+
+ private static final class ErrorResponseStatusCodeMatcher<T>
+ extends TypeSafeDiagnosingMatcher<CompletableFuture<T>> {
+
+ private final HttpResponseStatus expectedErrorResponse;
+
+ ErrorResponseStatusCodeMatcher(HttpResponseStatus expectedErrorResponse) {
+ this.expectedErrorResponse = expectedErrorResponse;
+ }
+
+ @Override
+ protected boolean matchesSafely(
+ CompletableFuture<T> future, Description mismatchDescription) {
+ try {
+ future.get();
+ throw new Error();
Review comment:
Some message would be nice - something like `Expected the request to fail with status code X, but it succeeded`
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/HandlerRequest.java
##########
@@ -100,7 +100,8 @@ public HandlerRequest(
+ value
+ "\".");
}
-
+ }
+ if (pathParameter.isResolved()) {
Review comment:
Just so I understand correctly: This covers cases where a parameter is resolved through other means than an explicitly specified value in the request, right? (for example by https://github.com/apache/flink/blob/master/flink-clients/src/main/java/org/apache/flink/client/program/rest/RestClusterClient.java#L440-L442)
Since the linked code was only refactored in this PR: Was this a bug before the change in this line and the parameters were ignored?
##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/rest/RestMatchers.java
##########
@@ -0,0 +1,83 @@
+/*
+ * 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;
+
+import org.apache.flink.runtime.rest.handler.RestHandlerException;
+
+import org.apache.flink.shaded.netty4.io.netty.handler.codec.http.HttpResponseStatus;
+
+import org.hamcrest.Description;
+import org.hamcrest.Matcher;
+import org.hamcrest.TypeSafeDiagnosingMatcher;
+
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutionException;
+
+/** Hamcrest matchers for REST handlers. */
+public class RestMatchers {
+
+ public static <T> Matcher<CompletableFuture<T>> respondsWithError(
+ HttpResponseStatus responseStatus) {
+ return new ErrorResponseStatusCodeMatcher<>(responseStatus);
+ }
+
+ private static final class ErrorResponseStatusCodeMatcher<T>
+ extends TypeSafeDiagnosingMatcher<CompletableFuture<T>> {
+
+ private final HttpResponseStatus expectedErrorResponse;
+
+ ErrorResponseStatusCodeMatcher(HttpResponseStatus expectedErrorResponse) {
+ this.expectedErrorResponse = expectedErrorResponse;
+ }
+
+ @Override
+ protected boolean matchesSafely(
+ CompletableFuture<T> future, Description mismatchDescription) {
+ try {
+ future.get();
+ throw new Error();
Review comment:
On second thought, this would provide nicer output for test failuers:
```
mismatchDescription.appendText("The request succeeded");
return false;
```
##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/dispatcher/DispatcherTest.java
##########
@@ -371,6 +373,78 @@ public void testCancellationDuringInitialization() throws Exception {
is(ApplicationStatus.CANCELED));
}
+ @Test
+ public void testCancellationOfCanceledTerminalDoesNotThrowException() throws Exception {
+ final CompletableFuture<JobManagerRunnerResult> jobTerminationFuture =
+ new CompletableFuture<>();
+ dispatcher =
+ createAndStartDispatcher(
+ heartbeatServices,
+ haServices,
+ new FinishingJobManagerRunnerFactory(jobTerminationFuture, () -> {}));
+ jobMasterLeaderElectionService.isLeader(UUID.randomUUID());
+ DispatcherGateway dispatcherGateway = dispatcher.getSelfGateway(DispatcherGateway.class);
+
+ JobID jobId = jobGraph.getJobID();
+
+ dispatcherGateway.submitJob(jobGraph, TIMEOUT).get();
+ jobTerminationFuture.complete(
+ JobManagerRunnerResult.forSuccess(
+ new ExecutionGraphInfo(
+ new ArchivedExecutionGraphBuilder()
+ .setJobID(jobId)
+ .setState(JobStatus.CANCELED)
+ .build())));
+
+ // wait for job to finish
+ dispatcherGateway.requestJobResult(jobId, TIMEOUT).get();
+ // sanity check
+ assertThat(
Review comment:
Not sure if we want to change this, but `Assert.assertThat` is deprecated, we should probably use `MatcherAssert.assertThat` instead (that it calls anyway). If we want to go through with this, a separate PR would probably make more sense that changes all the imports
--
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