[FLINK-7534] Create LegacyRestHandlerAdapter for old REST handlers #4603

tillrohrmann · 2017-08-26T20:27:10Z

What is the purpose of the change

Introduce LegacyRestHandler interface which the old REST handler have to implement
in order to make them usable for the RestServerEndpoint in combination with the
LegacyRestHandlerAdapter. The LegacyRestHandlerAdapter extends the AbstractRestHandler
and runs the LegacyRestHandler implementation.

As an example, this commit ports the ClusterOverviewHandler to the new interface. The
Dispatcher side still has to be properly implemented.

Brief change log

Introduce LegacyRestHandler interface to be implemented by the old REST handler
Implement LegacyRestHandlerAdapter which allows the LegacyRestHandler to be executed by the RestServerEndpoint
Port the ClusterOverviewHandler to the LegacyRestHandler interface

Verifying this change

This change has been manually verified by starting the DispatcherRestEndpoint and opening the web frontend in a browser.

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)

Documentation

Does this pull request introduce a new feature? (no)
If yes, how is the feature documented? (not applicable)

zentol · 2017-09-18T12:45:49Z

flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/RedirectHandler.java

+
+			} catch (Throwable throwable) {
+				logger.warn("Error occurred while processing web request.", throwable);
+					HandlerUtils.sendErrorResponse(


indentation is off

Good catch.

zentol · 2017-09-18T12:57:03Z

...me/src/main/java/org/apache/flink/runtime/messages/webmonitor/StatusOverviewWithVersion.java

+/**
+ * Status overview message including the current flink version and commit id.
+ */
+public class StatusOverviewWithVersion extends StatusOverview implements ResponseBody {


I suggest to add a test to verify that a given Request-/ResponseBody can be mapped to JSON and back again. An example can be found here: https://github.com/zentol/flink/blob/7072/flink-runtime/src/test/java/org/apache/flink/runtime/rest/messages/JobSubmitRequestBodyResponseTest.java

(We can probably create a simple test base for this)

Good point. I'll add it.

zentol · 2017-09-18T13:01:23Z

flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/LegacyRequest.java

+/**
+ * Request for Flink's legacy REST handler.
+ */
+public class LegacyRequest implements RequestBody {


Let's rename this to EmptyRequestBody, similar to EmptyMessageParameters.

zentol · 2017-09-18T13:04:24Z

flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/LegacyRestHandler.java

+
+	CompletableFuture<R> handleRequest(HandlerRequest<LegacyRequest, M> request, T gateway);
+
+	String[] getPaths();


I don't think we'll need this anymore, as this should be subsumed by the message headers.

True, I will remove it.

tzulitai

Changes on f8ef066 look good to me. Only some very minor typo nitpicks.

tzulitai · 2017-09-20T07:09:07Z

flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/LegacyRestHandler.java

+import java.util.concurrent.CompletableFuture;
+
+/**
+ * Interface which Flink's legacy REST handler have to implement in order to bu usable


typo: bu -> be?

good catch.

tzulitai · 2017-09-20T07:10:10Z

...me/src/main/java/org/apache/flink/runtime/messages/webmonitor/StatusOverviewWithVersion.java

+import java.util.Objects;
+
+/**
+ * Status overview message including the current flink version and commit id.


nit: Capital F for Flink

tzulitai · 2017-09-20T07:58:22Z

flink-runtime/src/main/java/org/apache/flink/runtime/rest/messages/ClusterOverviewHeaders.java

+
+	@Override
+	public String getTargetRestEndpointURL() {
+		return "/overview";


Maybe we should make ClusterOverviewHandler#CLUSTER_OVERVIEW_REST_PATH public and reuse that

Will move CLUSTER_OVERVIEW_REST_PATH to ClusterOverviewHeaders and reference it from ClusterOverviewHandler.

tillrohrmann · 2017-09-20T08:37:11Z

Thanks for your review @tzulitai. I will address your comments and then merge this PR.

Introduce LegacyRestHandler interface which the old REST handler have to implement in order to make them usable for the RestServerEndpoint in combination with the LegacyRestHandlerAdapter. The LegacyRestHandlerAdapter extends the AbstractRestHandler and runs the LegacyRestHandler implementation. As an example, this commit ports the ClusterOverviewHandler to the new interface. The Dispatcher side still has to be properly implemented.

tillrohrmann force-pushed the createLegacyHandlerAdapter branch from e809638 to 7e3d861 Compare August 26, 2017 21:49

tillrohrmann force-pushed the createLegacyHandlerAdapter branch 5 times, most recently from 5eb55a2 to 3bae198 Compare September 6, 2017 11:40

zentol requested changes Sep 18, 2017

View reviewed changes

tillrohrmann force-pushed the createLegacyHandlerAdapter branch from 3bae198 to f8ef066 Compare September 19, 2017 13:39

tzulitai approved these changes Sep 20, 2017

View reviewed changes

tzulitai reviewed Sep 20, 2017

View reviewed changes

tillrohrmann force-pushed the createLegacyHandlerAdapter branch 2 times, most recently from ae7f59f to 1991c9e Compare September 20, 2017 09:20

tillrohrmann force-pushed the createLegacyHandlerAdapter branch from 1991c9e to d794ac0 Compare September 20, 2017 12:31

asfgit closed this in dbabdb1 Sep 20, 2017

tillrohrmann deleted the createLegacyHandlerAdapter branch September 21, 2017 07:41

rmetzger added component=Runtime/WebFrontend component=Runtime/REST labels Mar 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-7534] Create LegacyRestHandlerAdapter for old REST handlers #4603

[FLINK-7534] Create LegacyRestHandlerAdapter for old REST handlers #4603

tillrohrmann commented Aug 26, 2017

zentol Sep 18, 2017

tillrohrmann Sep 19, 2017

zentol Sep 18, 2017

tillrohrmann Sep 19, 2017

zentol Sep 18, 2017

tillrohrmann Sep 19, 2017

zentol Sep 18, 2017

tillrohrmann Sep 19, 2017

tzulitai left a comment •

edited

tzulitai Sep 20, 2017

tillrohrmann Sep 20, 2017

tzulitai Sep 20, 2017

tillrohrmann Sep 20, 2017

tzulitai Sep 20, 2017

tillrohrmann Sep 20, 2017

tillrohrmann commented Sep 20, 2017


		CompletableFuture<R> handleRequest(HandlerRequest<LegacyRequest, M> request, T gateway);

		String[] getPaths();

[FLINK-7534] Create LegacyRestHandlerAdapter for old REST handlers #4603

[FLINK-7534] Create LegacyRestHandlerAdapter for old REST handlers #4603

Conversation

tillrohrmann commented Aug 26, 2017

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzulitai left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tillrohrmann commented Sep 20, 2017

tzulitai left a comment •

edited