You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by tzulitai <gi...@git.apache.org> on 2017/09/20 17:21:28 UTC
[GitHub] flink pull request #4691: [FLINK-7647] [flip6] Port JobManagerConfigHandler ...
GitHub user tzulitai opened a pull request:
https://github.com/apache/flink/pull/4691
[FLINK-7647] [flip6] Port JobManagerConfigHandler to new REST endpoint
## What is the purpose of the change
This PR ports the existing `JobManagerConfigHandler` to the new REST endpoint. This includes introducing the `ClusterConfiguration` response and `ClusterConfigurationHeaders`. The `DispatcherRestEndpoint` now registers the `JobManagerConfigHandler`.
Additionally, this PR also contains other cosmetic changes, such as properly renaming the `JobManagerConfigHandler` to `ClusterConfigHandler`, and refactoring common test logic for marshalling / unmarshalling of REST responses.
## Brief change log
- Let `JobManagerConfigHandler` implement `LegacyRestEHandler`
- Register `JobManagerConfigHandler` at `DispatcherRestEndpoint`
- Rename `JobManagerConfigHandler` to `ClusterConfigHandler`
- Introduce `RestResponseMarshallingTestBase`
## Verifying this change
This change is a trivial rework / code cleanup.
Only additional test is `ClusterConfigurationTest` for the marshalling of the `ClusterConfiguration`.
## 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: yes
## 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/tzulitai/flink portJobManagerConfigHandler
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/4691.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 #4691
----
commit bcfdd1ed3f3b9d4cd29e694c65d30a6711ee9d58
Author: Till Rohrmann <tr...@apache.org>
Date: 2017-08-21T13:11:08Z
[FLINK-7535] Port DashboardConfigHandler to new REST endpoint
Lets DashboardConfigHandler implement the LegacyRestHandler. Moreover, this
commit defines the appropriate DashboardConfigurationHeaders.
The DispatcherRestEndpoint registers the DashboardConfigHandler.
This closes #4604.
commit ade082fbb27419a8e1a18d0086be267a3721e598
Author: Tzu-Li (Gordon) Tai <tz...@apache.org>
Date: 2017-09-20T16:25:28Z
[FLINK-7647] [flip6] Port JobManagerConfigHandler to new REST endpoint
This commit lets the JobManagerConfigHandler implement the
LegacyRestHandler interface in order to be ported to the new REST
endpoint. This includes the introduction of ClusterConfiguration
response body and ClusterConfigurationHeaders.
The DispatcherRestEndpoint now also registers the
JobManagerConfigHandler.
commit 890faba271194a49fe9e5efea81917b236fc3b0a
Author: Tzu-Li (Gordon) Tai <tz...@apache.org>
Date: 2017-09-20T16:51:27Z
[FLINK-7647] [flip6] Rename JobManagerConfigHandler to ClusterConfigHandler
The original naming wouldn't make sense for the FLIP-6 redesign, since
we would have multiple per-job JobManagers for each cluster, which
shares the same configuration.
The REST path is still left untouched and not part of this commit, as
that would involve more changes in flink-runtime-web.
commit 16902cc8775b03f79770a998873dc0786b73264f
Author: Tzu-Li (Gordon) Tai <tz...@apache.org>
Date: 2017-09-20T17:07:11Z
[FLINK-7647] [flip6] Introduce test base for REST response marshalling
Introduces a common test base that for all REST responses, a subclass
should be implemented to verify that the response can be correctly
marshalled and unmarshalled.
----
---
[GitHub] flink pull request #4691: [FLINK-7647] [flip6] Port JobManagerConfigHandler ...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/4691#discussion_r140379775
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/dispatcher/DispatcherRestEndpoint.java ---
@@ -55,16 +59,19 @@
public class DispatcherRestEndpoint extends RestServerEndpoint {
private final GatewayRetriever<DispatcherGateway> leaderRetriever;
+ private final Configuration clusterConfiguration;
private final RestHandlerConfiguration restConfiguration;
private final Executor executor;
public DispatcherRestEndpoint(
- RestServerEndpointConfiguration configuration,
+ Configuration clusterConfiguration,
+ RestServerEndpointConfiguration endpointConfiguration,
--- End diff --
Shall we remove the order between `clusterConfiguration` and `endpointConfiguration`? I usually like to have the super class arguments first.
---
[GitHub] flink issue #4691: [FLINK-7647] [flip6] Port JobManagerConfigHandler to new ...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/4691
Thanks for your contribution @tzulitai. There are still some checkstyle violations in the code.
---
[GitHub] flink pull request #4691: [FLINK-7647] [flip6] Port JobManagerConfigHandler ...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/4691#discussion_r140381079
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/messages/ClusterConfiguration.java ---
@@ -0,0 +1,57 @@
+/*
+ * 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.handler.legacy.messages;
+
+import org.apache.flink.configuration.Configuration;
+import org.apache.flink.runtime.rest.handler.legacy.ClusterConfigHandler;
+import org.apache.flink.runtime.rest.messages.ResponseBody;
+
+import java.util.ArrayList;
+
+/**
+ * Response of the {@link ClusterConfigHandler}, respresented as a list
+ * of key-value pairs of the cluster {@link Configuration}.
+ */
+public class ClusterConfiguration extends ArrayList<ClusterConfigurationEntry> implements ResponseBody {
--- End diff --
Serial version uid missing.
---
[GitHub] flink pull request #4691: [FLINK-7647] [flip6] Port JobManagerConfigHandler ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/4691
---
[GitHub] flink pull request #4691: [FLINK-7647] [flip6] Port JobManagerConfigHandler ...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/4691#discussion_r140381187
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/messages/ClusterConfiguration.java ---
@@ -0,0 +1,57 @@
+/*
+ * 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.handler.legacy.messages;
+
+import org.apache.flink.configuration.Configuration;
+import org.apache.flink.runtime.rest.handler.legacy.ClusterConfigHandler;
+import org.apache.flink.runtime.rest.messages.ResponseBody;
+
+import java.util.ArrayList;
+
+/**
+ * Response of the {@link ClusterConfigHandler}, respresented as a list
+ * of key-value pairs of the cluster {@link Configuration}.
+ */
+public class ClusterConfiguration extends ArrayList<ClusterConfigurationEntry> implements ResponseBody {
--- End diff --
There is already another class called `ClusterConfiguration`. Maybe we should rename it in order to disambiguate it.
---
[GitHub] flink pull request #4691: [FLINK-7647] [flip6] Port JobManagerConfigHandler ...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:
https://github.com/apache/flink/pull/4691#discussion_r141055340
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/messages/ClusterConfiguration.java ---
@@ -0,0 +1,57 @@
+/*
+ * 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.handler.legacy.messages;
+
+import org.apache.flink.configuration.Configuration;
+import org.apache.flink.runtime.rest.handler.legacy.ClusterConfigHandler;
+import org.apache.flink.runtime.rest.messages.ResponseBody;
+
+import java.util.ArrayList;
+
+/**
+ * Response of the {@link ClusterConfigHandler}, respresented as a list
+ * of key-value pairs of the cluster {@link Configuration}.
+ */
+public class ClusterConfiguration extends ArrayList<ClusterConfigurationEntry> implements ResponseBody {
--- End diff --
I'll rename this to `ClusterConfigurationInfo`.
---
[GitHub] flink pull request #4691: [FLINK-7647] [flip6] Port JobManagerConfigHandler ...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:
https://github.com/apache/flink/pull/4691#discussion_r141047777
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/messages/ClusterConfiguration.java ---
@@ -0,0 +1,57 @@
+/*
+ * 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.handler.legacy.messages;
+
+import org.apache.flink.configuration.Configuration;
+import org.apache.flink.runtime.rest.handler.legacy.ClusterConfigHandler;
+import org.apache.flink.runtime.rest.messages.ResponseBody;
+
+import java.util.ArrayList;
+
+/**
+ * Response of the {@link ClusterConfigHandler}, respresented as a list
+ * of key-value pairs of the cluster {@link Configuration}.
+ */
+public class ClusterConfiguration extends ArrayList<ClusterConfigurationEntry> implements ResponseBody {
--- End diff --
Yes, I found the original use of the JSONArray structure very strange also.
We can include a change for this probably when we also re-work the REST resource path for this handler.
---
[GitHub] flink issue #4691: [FLINK-7647] [flip6] Port JobManagerConfigHandler to new ...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/4691
@tillrohrmann I've corrected checkstyles violations and rebased.
---
[GitHub] flink issue #4691: [FLINK-7647] [flip6] Port JobManagerConfigHandler to new ...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/4691
Sorry about that, local Travis tests pass, should be fine now.
---
[GitHub] flink pull request #4691: [FLINK-7647] [flip6] Port JobManagerConfigHandler ...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:
https://github.com/apache/flink/pull/4691#discussion_r141047294
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/dispatcher/DispatcherRestEndpoint.java ---
@@ -55,16 +59,19 @@
public class DispatcherRestEndpoint extends RestServerEndpoint {
private final GatewayRetriever<DispatcherGateway> leaderRetriever;
+ private final Configuration clusterConfiguration;
private final RestHandlerConfiguration restConfiguration;
private final Executor executor;
public DispatcherRestEndpoint(
- RestServerEndpointConfiguration configuration,
+ Configuration clusterConfiguration,
+ RestServerEndpointConfiguration endpointConfiguration,
--- End diff --
Will change.
---
[GitHub] flink issue #4691: [FLINK-7647] [flip6] Port JobManagerConfigHandler to new ...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on the issue:
https://github.com/apache/flink/pull/4691
Thanks a lot for the review @tillrohrmann. I'll go ahead and merge this PR after addressing your comments.
---
[GitHub] flink pull request #4691: [FLINK-7647] [flip6] Port JobManagerConfigHandler ...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/4691#discussion_r140382392
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/messages/ClusterConfiguration.java ---
@@ -0,0 +1,57 @@
+/*
+ * 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.handler.legacy.messages;
+
+import org.apache.flink.configuration.Configuration;
+import org.apache.flink.runtime.rest.handler.legacy.ClusterConfigHandler;
+import org.apache.flink.runtime.rest.messages.ResponseBody;
+
+import java.util.ArrayList;
+
+/**
+ * Response of the {@link ClusterConfigHandler}, respresented as a list
+ * of key-value pairs of the cluster {@link Configuration}.
+ */
+public class ClusterConfiguration extends ArrayList<ClusterConfigurationEntry> implements ResponseBody {
--- End diff --
I'm actually wondering why we don't extend from a `Map` implementation since it should behave more like a map. This of course would require changes on the web gui side. I think now, the JSON would like
```
[{"key":"keyvalue", "value":"valuevalue"}, {....}]
```
instead of
```
{"keyvalue":"valuevalue", ....}
```
Maybe we could change this in the future.
---
[GitHub] flink pull request #4691: [FLINK-7647] [flip6] Port JobManagerConfigHandler ...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/4691#discussion_r140380156
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/ClusterConfigHandler.java ---
@@ -33,20 +43,27 @@
/**
* Returns the Job Manager's configuration.
*/
-public class JobManagerConfigHandler extends AbstractJsonRequestHandler {
-
- private static final String JOBMANAGER_CONFIG_REST_PATH = "/jobmanager/config";
+public class ClusterConfigHandler extends AbstractJsonRequestHandler
+ implements LegacyRestHandler<DispatcherGateway, ClusterConfiguration, EmptyMessageParameters> {
private final Configuration config;
- public JobManagerConfigHandler(Executor executor, Configuration config) {
+ public ClusterConfigHandler(Executor executor, Configuration config) {
super(executor);
- this.config = config;
+ this.config = Preconditions.checkNotNull(config);
}
@Override
public String[] getPaths() {
- return new String[]{JOBMANAGER_CONFIG_REST_PATH};
+ return new String[]{ClusterConfigurationHeaders.CLUSTER_CONFIG_REST_PATH};
+ }
+
+ @Override
+ public CompletableFuture<ClusterConfiguration> handleRequest(
+ HandlerRequest<EmptyRequestBody, EmptyMessageParameters> request,
+ DispatcherGateway gateway) {
+
+ return CompletableFuture.supplyAsync(() -> ClusterConfiguration.from(config), executor);
--- End diff --
Maybe we could generate the `ClusterConfiguration` instance once at creation time and then always return this element as a `CompletableFuture.completedFuture(clusterConfiguration)`.
---
[GitHub] flink issue #4691: [FLINK-7647] [flip6] Port JobManagerConfigHandler to new ...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the issue:
https://github.com/apache/flink/pull/4691
Still failing with `ClusterConfigHandlerTest.testGetPaths:32 NullPointer`
---
[GitHub] flink pull request #4691: [FLINK-7647] [flip6] Port JobManagerConfigHandler ...
Posted by tzulitai <gi...@git.apache.org>.
Github user tzulitai commented on a diff in the pull request:
https://github.com/apache/flink/pull/4691#discussion_r141047390
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/legacy/ClusterConfigHandler.java ---
@@ -33,20 +43,27 @@
/**
* Returns the Job Manager's configuration.
*/
-public class JobManagerConfigHandler extends AbstractJsonRequestHandler {
-
- private static final String JOBMANAGER_CONFIG_REST_PATH = "/jobmanager/config";
+public class ClusterConfigHandler extends AbstractJsonRequestHandler
+ implements LegacyRestHandler<DispatcherGateway, ClusterConfiguration, EmptyMessageParameters> {
private final Configuration config;
- public JobManagerConfigHandler(Executor executor, Configuration config) {
+ public ClusterConfigHandler(Executor executor, Configuration config) {
super(executor);
- this.config = config;
+ this.config = Preconditions.checkNotNull(config);
}
@Override
public String[] getPaths() {
- return new String[]{JOBMANAGER_CONFIG_REST_PATH};
+ return new String[]{ClusterConfigurationHeaders.CLUSTER_CONFIG_REST_PATH};
+ }
+
+ @Override
+ public CompletableFuture<ClusterConfiguration> handleRequest(
+ HandlerRequest<EmptyRequestBody, EmptyMessageParameters> request,
+ DispatcherGateway gateway) {
+
+ return CompletableFuture.supplyAsync(() -> ClusterConfiguration.from(config), executor);
--- End diff --
That makes a lot of sense, yes. Will change.
---