[GitHub] flink pull request #5430: [FLINK-8605] [rest] Enable job cancellation from t...
Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/5430 ---
[GitHub] flink pull request #5430: [FLINK-8605] [rest] Enable job cancellation from t...
Github user GJL commented on a diff in the pull request: https://github.com/apache/flink/pull/5430#discussion_r168420232 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/webmonitor/WebMonitorEndpoint.java --- @@ -593,6 +617,8 @@ public void shutdown(Time timeout) { } catch (Exception e) { log.warn("Error while stopping leaderElectionService", e); } + + super.shutdown(timeout); --- End diff -- ok ---
[GitHub] flink pull request #5430: [FLINK-8605] [rest] Enable job cancellation from t...
Github user tillrohrmann commented on a diff in the pull request: https://github.com/apache/flink/pull/5430#discussion_r168187766 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/JobTerminationHeaders.java --- @@ -32,7 +32,7 @@ private static final JobTerminationHeaders INSTANCE = new JobTerminationHeaders(); - private JobTerminationHeaders() {} + protected JobTerminationHeaders() {} --- End diff -- True, will correct it. ---
[GitHub] flink pull request #5430: [FLINK-8605] [rest] Enable job cancellation from t...
Github user tillrohrmann commented on a diff in the pull request: https://github.com/apache/flink/pull/5430#discussion_r168187481 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/webmonitor/WebMonitorEndpoint.java --- @@ -593,6 +617,8 @@ public void shutdown(Time timeout) { } catch (Exception e) { log.warn("Error while stopping leaderElectionService", e); } + + super.shutdown(timeout); --- End diff -- Because `leaderElectionService.stop()` can call `revokeLeadership`. Inside of `revokeLeadership` we call `RestServerEndpoint#getRestAddress` which cannot be accessed after it has been shut down. This happens when calling `super.shutdown`. Moreover, I think it is a good idea to call the close method first on the sub class and then on the parent class in order to guarantee that you don't close any services from the parent class which the sub class might still need for closing. ---
[GitHub] flink pull request #5430: [FLINK-8605] [rest] Enable job cancellation from t...
Github user tillrohrmann commented on a diff in the pull request: https://github.com/apache/flink/pull/5430#discussion_r168186153 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/YarnCancelJobTerminationHeaders.java --- @@ -0,0 +1,55 @@ +/* + * 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; + +import org.apache.flink.runtime.rest.HttpMethodWrapper; +import org.apache.flink.runtime.rest.handler.RestHandlerSpecification; +import org.apache.flink.runtime.rest.handler.job.JobTerminationHandler; + +/** + * {@link RestHandlerSpecification} for the {@link JobTerminationHandler} which is registered for + * compatibility with the Yarn proxy as a GET call. + * + * @see https://issues.apache.org/jira/browse/YARN-2031";>YARN-2031. --- End diff -- Good point, will change it :-) ---
[GitHub] flink pull request #5430: [FLINK-8605] [rest] Enable job cancellation from t...
Github user GJL commented on a diff in the pull request: https://github.com/apache/flink/pull/5430#discussion_r168166945 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/YarnCancelJobTerminationHeaders.java --- @@ -0,0 +1,55 @@ +/* + * 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; + +import org.apache.flink.runtime.rest.HttpMethodWrapper; +import org.apache.flink.runtime.rest.handler.RestHandlerSpecification; +import org.apache.flink.runtime.rest.handler.job.JobTerminationHandler; + +/** + * {@link RestHandlerSpecification} for the {@link JobTerminationHandler} which is registered for + * compatibility with the Yarn proxy as a GET call. + * + * @see https://issues.apache.org/jira/browse/YARN-2031";>YARN-2031. --- End diff -- nit: should be `* @see https://issues.apache.org/jira/browse/YARN-2031";>YARN-2031` ---
[GitHub] flink pull request #5430: [FLINK-8605] [rest] Enable job cancellation from t...
Github user GJL commented on a diff in the pull request: https://github.com/apache/flink/pull/5430#discussion_r168148700 --- Diff: flink-runtime/src/test/java/org/apache/flink/runtime/webmonitor/TestingRestfulGateway.java --- @@ -169,22 +192,26 @@ public static Builder newBuilder() { private String address = LOCALHOST; private String hostname = LOCALHOST; private String restAddress = LOCALHOST; + private Function> cancelJobFunction; + private Function> stopJobFunction; private Function> requestJobFunction; private Function> requestJobStatusFunction; private Supplier> requestMultipleJobDetailsSupplier; private Supplier> requestClusterOverviewSupplier; private Supplier>> requestMetricQueryServicePathsSupplier; private Supplier>>> requestTaskManagerMetricQueryServicePathsSupplier; - private BiFunction> requestOeratorBackPressureStatsFunction; + private BiFunction> requestOperatorBackPressureStatsFunction; --- End diff -- nice ---
[GitHub] flink pull request #5430: [FLINK-8605] [rest] Enable job cancellation from t...
Github user GJL commented on a diff in the pull request: https://github.com/apache/flink/pull/5430#discussion_r168149188 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/JobTerminationHeaders.java --- @@ -32,7 +32,7 @@ private static final JobTerminationHeaders INSTANCE = new JobTerminationHeaders(); - private JobTerminationHeaders() {} + protected JobTerminationHeaders() {} --- End diff -- I don't think this is needed. ---
[GitHub] flink pull request #5430: [FLINK-8605] [rest] Enable job cancellation from t...
Github user GJL commented on a diff in the pull request: https://github.com/apache/flink/pull/5430#discussion_r168165205 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/webmonitor/WebMonitorEndpoint.java --- @@ -593,6 +617,8 @@ public void shutdown(Time timeout) { } catch (Exception e) { log.warn("Error while stopping leaderElectionService", e); } + + super.shutdown(timeout); --- End diff -- Why is it better to move it to the end? ---
[GitHub] flink pull request #5430: [FLINK-8605] [rest] Enable job cancellation from t...
GitHub user tillrohrmann opened a pull request: https://github.com/apache/flink/pull/5430 [FLINK-8605] [rest] Enable job cancellation from the web UI ## What is the purpose of the change In order to support the job cancellation from the web UI, including when using Yarn, we have to register the JobTerminationHandler under /jobs/:jobid/yarn-cancel and /jobs/:jobid/yarn-stop. This is just a temporary fix until we can send arbitrary REST verbs through the Yarn proxy. ## Brief change log - Introduce `YarnCancelJobTerminationHeaders` - Introduce `YarnStopJobTerminationHeaders` - Register `JobTerminationHandler` with default `TerminationMode#CANCEL` under `YarnCancelJobTerminationHeaders` - Register `JobTerminationHandler` with default `TerminationMode#STOP` under `YarnStopJobTerminationHeaders` ## Verifying this change - Added `YarnCancelJobTerminationHeadersTest` and `YarnStopJobTerminationHeadersTest` - Tested manually ## Does this pull request potentially affect one of the following parts: - Dependencies (does it add or upgrade a dependency): (no) - The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (no) - The serializers: (no) - The runtime per-record code paths (performance sensitive): (no) - Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (no) - The S3 file system connector: (no) ## Documentation - Does this pull request introduce a new feature? (no) - If yes, how is the feature documented? (not applicable) You can merge this pull request into a Git repository by running: $ git pull https://github.com/tillrohrmann/flink enableWebJobCancellation Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/5430.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #5430 commit 216876ae1d3fd0509bd5591576d9d4c960c1aa80 Author: Till Rohrmann Date: 2018-02-06T18:11:53Z [hotfix] Change shutdown order in WebMonitorEndpoint to avoid illegal state commit f906423456b0f42c46213584b91d7c75321289c5 Author: Till Rohrmann Date: 2018-02-08T12:43:09Z [FLINK-8604] [rest] Move JobTerminationHandler into WebMonitorEndpoint Register the JobTerminationHandler at the WebMonitorEndpoint to make it accessible to all REST endpoints. commit e5a80cc4a2c676c08de65cc1d68c2e4937b5f499 Author: Till Rohrmann Date: 2018-02-07T11:35:58Z [FLINK-8605] [rest] Enable job cancellation from the web UI In order to support the job cancellation from the web UI, including when using Yarn, we have to register the JobTerminationHandler under /jobs/:jobid/yarn-cancel and /jobs/:jobid/yarn-stop. This is just a temporary fix until we can send arbitrary REST verbs through the Yarn proxy. ---