[FLINK-7647] [flip6] Port JobManagerConfigHandler to new REST endpoint #4691
Conversation
|
Thanks for your contribution @tzulitai. There are still some checkstyle violations in the code. |
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.
tzulitai
force-pushed
the
portJobManagerConfigHandler
branch
from
16902cc
to
288d974
Compare
|
@tillrohrmann I've corrected checkstyles violations and rebased. |
…andler 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.
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.
tzulitai
force-pushed
the
portJobManagerConfigHandler
branch
from
288d974
to
642e488
Compare
|
Still failing with |
|
Sorry about that, local Travis tests pass, should be fine now. |
There was a problem hiding this comment.
Changes look good to me @tzulitai. I had only minor comments. After addressing them, we can merge this PR :-)
| private final RestHandlerConfiguration restConfiguration; | ||
| private final Executor executor; | ||
|
|
||
| public DispatcherRestEndpoint( | ||
| RestServerEndpointConfiguration configuration, | ||
| Configuration clusterConfiguration, | ||
| RestServerEndpointConfiguration endpointConfiguration, |
There was a problem hiding this comment.
Shall we remove the order between clusterConfiguration and endpointConfiguration? I usually like to have the super class arguments first.
| HandlerRequest<EmptyRequestBody, EmptyMessageParameters> request, | ||
| DispatcherGateway gateway) { | ||
|
|
||
| return CompletableFuture.supplyAsync(() -> ClusterConfiguration.from(config), executor); |
There was a problem hiding this comment.
Maybe we could generate the ClusterConfiguration instance once at creation time and then always return this element as a CompletableFuture.completedFuture(clusterConfiguration).
There was a problem hiding this comment.
That makes a lot of sense, yes. Will change.
| * 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 { |
There was a problem hiding this comment.
Serial version uid missing.
| * 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 { |
There was a problem hiding this comment.
There is already another class called ClusterConfiguration. Maybe we should rename it in order to disambiguate it.
There was a problem hiding this comment.
I'll rename this to ClusterConfigurationInfo.
| * 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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Thanks a lot for the review @tillrohrmann. I'll go ahead and merge this PR after addressing your comments. |
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. This closes apache#4691. This closes apache#4720.
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. This closes apache#4691. This closes apache#4720.
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. This closes apache#4691. This closes apache#4720.
What is the purpose of the change
This PR ports the existing
JobManagerConfigHandlerto the new REST endpoint. This includes introducing theClusterConfigurationresponse andClusterConfigurationHeaders. TheDispatcherRestEndpointnow registers theJobManagerConfigHandler.Additionally, this PR also contains other cosmetic changes, such as properly renaming the
JobManagerConfigHandlertoClusterConfigHandler, and refactoring common test logic for marshalling / unmarshalling of REST responses.Brief change log
JobManagerConfigHandlerimplementLegacyRestEHandlerJobManagerConfigHandleratDispatcherRestEndpointJobManagerConfigHandlertoClusterConfigHandlerRestResponseMarshallingTestBaseVerifying this change
This change is a trivial rework / code cleanup.
Only additional test is
ClusterConfigurationTestfor the marshalling of theClusterConfiguration.Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation