[FLINK-9386] Embed netty router #6031

pnowojski · 2018-05-17T10:24:47Z

This replaces netty-router dependency with our own version of it, which is simplified and adds guarantees about order of matching router patterns.

This is a prerequisite for https://issues.apache.org/jira/browse/FLINK-3952 . netty-router 1.10 is incompatible with Netty 4.1, while netty-router 2.2.0 brakes a compatibility in a way that we were unable to use it.

Verifying this change

This change ports part of the netty-router code, including tests and adds a test for preserving patter matching order by the Router. Additionally this change is covered by various of our ITCases.

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

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
The serializers: (yes / no / don't know)
The runtime per-record code paths (performance sensitive): (yes / no / don't know)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (yes / no / don't know)
The S3 file system connector: (yes / no / don't know)

Documentation

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

uce

I checked the copied classes and compared them to the originals. The simplications/merges look good to me and make a lot of sense. I also like the renamings. 👍
I've manually checked that registration order is respected. We still need to manually make sure that we register the handlers in the correct order. In the legacy WebRuntimeMonitor this was accidental as far as I can tell. In the new RestServerEndpoint we explicitly sort them before registering:
```
Collections.sort(handlers, RestHandlerUrlComparator.INSTANCE);
```
I'm wondering whether we should make this fact more explicit by adding comments to the add* methods in Router. Another approach is the Routes builder I mentioned as an inline comment.
I've furthermore build the project and check that job submission via REST and other endpoints for the web UI work as expected.
Do we file a follow up ticket to remove netty-router from our shaded Netty? Optionally, we might consider adding an import suppression for org.apache.flink.shaded.netty4.io.netty.handler.codec.http.router.* to guard against any accidental usage until we remove the dependency.
I don't know what your opinion is on cleaning up the copied classes. I'm OK with keeping everything as is, but I get a bunch of minor/trivial warnings about the code (like code visibility, unchecked calls, unused variables, etc.). I understand if we want to stay close to the copied classes.

uce · 2018-05-23T09:47:35Z

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

+import static java.util.Objects.requireNonNull;
+
+/**
+ * This class replaces the standard error response to be identical with those sent by the {@link AbstractRestHandler}.


I would add a comment that this was copied and simplified from https://github.com/sinetja/netty-router/blob/1.10/src/main/java/io/netty/handler/codec/http/router/Handler.java and https://github.com/sinetja/netty-router/blob/1.10/src/main/java/io/netty/handler/codec/http/router/AbstractHandler.java for future reference. That can be beneficial in the future.

I would also copy the high-level comment from that class: Inbound handler that converts HttpRequest to RoutedRequest and passes RoutedRequest to the matched handler as found in the original Handler class.

uce · 2018-05-23T10:02:16Z

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

+/**
+ * This class replaces the standard error response to be identical with those sent by the {@link AbstractRestHandler}.
+ */
+public class RouterHandler extends SimpleChannelInboundHandler<HttpRequest> {


I've verified that this class merges the behaviour of Handler and AbstractHandler. 👍

Since we copied the code, we might leave it as is, but I noticed the following minor things (there are similar warnings in the other copied classes):

L46, L47: fields can be private

L62: throws Exception can be removed

L98: I get a unchecked call warning for RouteResult. We could use <?> to get rid of it I think

uce · 2018-05-23T10:28:53Z

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

+import java.util.Map.Entry;
+
+/**
+ * This is adopted and simplified code from tv.cntt:netty-router library. For more information check {@link Router}.


Maybe add a link to https://github.com/sinetja/netty-router/blob/2.2.0/src/main/java/io/netty/handler/codec/http/router/MethodlessRouter.java and https://github.com/sinetja/netty-router/blob/2.2.0/src/main/java/io/netty/handler/codec/http/router/OrderlessRouter.java as a reference

uce · 2018-05-23T11:36:03Z

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

+	private static final Logger log = LoggerFactory.getLogger(MethodlessRouter.class);
+
+	// A path pattern can only point to one target
+	private final Map<PathPattern, T> routes = new LinkedHashMap<>();


Using the LinkedHashMap is what preserves the order of adding handlers, right? Or is there another thing that I've missed?

Please correct me if I'm wrong but I have a question regarding memory visibility: The thread that updates this map and the Netty event loop threads are different, so there might theoretically be a memory visibility issue if routes are added after the router has been passed to the RouterHandler. I don't think that we do this currently, but the API theoretically allows it. I'm wondering whether it makes sense to make the routes immutable, maybe something like creating the routes with a builder:

Routes routes = new RoutesBuilder() .addGet(...) ... .build(); Router router = new Router(routes);

Or use a thread-safe map here that also preserves ordering (maybe wrap using synchronizedMap).

Since we currently rely on correct registration order (e.g. /jobs/overview before /jobs/:id for correct matching), the immutable approach would allow us to include a utility in Routes that sorts pattern as done in RestServerEndpoint:143:

Collections.sort(handlers,RestHandlerUrlComparator.INSTANCE);

I'm not convinced that embedding the sorting logic inside router is good idea. Now it's clear that order of adding methods matter and it should be handled by the caller. If we provide sorting option, it can add confusion, especially because it can not be mandatory option - our sorting based on number of parameters is just a hack that kind of works with our patterns at the moment, but there are corner cases that would have to be "sorted"/"ordered" manually. Like patterns:

/jobs/foo/:B /jobs/:A/bar

Which one of it should be matched to url /jobs/foo/bar?

Regarding thread safety, this class is just simply not thread safe, and it should be the user's responsibility to handle it appropriately. On the other hand, turning it into immutable would hardcode some assumption and prevent some possible use cases and I'm not sure if that's worth the effort (it would add some substantial boiler plate code in storing intermediate builder's state, without clear benefits for the future since this code hasn't changed much for last couple of years). Don't get me wrong, the builder approach is nicer (regardless of sorting/immutability), but I'm just not sure if it's worth the extra effort.

Fine with me to not invest more time into this and keep it as is 👍

uce · 2018-05-23T11:43:00Z

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

+			}
+		}
+
+		channelHandlerContext.fireChannelRead(new RoutedRequest(routeResult, httpRequest).retain());


Do you think it makes sense to move new RoutedRequest(routeResult, httpRequest).retain() to a separate line? I was wondering whether it is retained as in the original handler, because I missed the retain() here.

I find the following clearer, but feel free to ignore:

RoutedRequest request = new RoutedRequest(routeResult, httpRequest); request.retain(); channelHandlerContext.fireChannelRead(request);

changed into

RoutedRequest<?> request = new RoutedRequest<>(routeResult, httpRequest); channelHandlerContext.fireChannelRead(request.retain());

I like having .retain() calls to be in the same line where they are actually designed for, since it's kind of self documenting which line is the reason behind retaining.

uce · 2018-05-23T13:30:21Z

flink-runtime-web/src/main/java/org/apache/flink/runtime/webmonitor/RuntimeMonitorHandler.java

 		CompletableFuture<FullHttpResponse> responseFuture;
+		RouteResult result = routedRequest.getRouteResult();


I think if you do RouteResult<?> result = routedRequest.getRouteResult(); you don't need the cast to Set<String> in lines 101 and 106.

uce · 2018-05-23T13:36:46Z

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

+ * <p>The pattern will be broken to tokens, example:
+ * {@code ["constant1", ":variable", "constant2", ":*"]}
+ */
+final class PathPattern {


Maybe add reference to https://github.com/sinetja/netty-router/blob/2.2.0/src/main/java/io/netty/handler/codec/http/router/PathPattern.java

uce · 2018-05-23T13:37:20Z

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

+ *
+ * <p>Result of calling {@link Router#route(HttpMethod, String)}.
+ */
+public class RouteResult<T> {


Maybe add reference to https://github.com/sinetja/netty-router/blob/2.2.0/src/main/java/io/netty/handler/codec/http/router/RouteResult.java

uce · 2018-05-23T13:37:54Z

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

+import java.util.Set;
+
+/**
+ * This is adopted and simplified code from tv.cntt:netty-router library. Compared to original version this one


Maybe add a link to https://github.com/sinetja/netty-router/blob/2.2.0/src/main/java/io/netty/handler/codec/http/router/Router.java

pnowojski · 2018-05-23T15:04:13Z

Thanks for the review and manual checks! I either addressed your comments in fixup commit and left comment responses otherwise.

Do we file a follow up ticket to remove netty-router from our shaded Netty? Optionally, we might consider adding an import suppression for org.apache.flink.shaded.netty4.io.netty.handler.codec.http.router.* to guard against any accidental usage until we remove the dependency.

I planned to upgrade netty and drop netty-router in one step of upgrading flink-shaded-netty. Do you think it should be split somehow?

I don't know what your opinion is on cleaning up the copied classes. I'm OK with keeping everything as is, but I get a bunch of minor/trivial warnings about the code (like code visibility, unchecked calls, unused variables, etc.). I understand if we want to stay close to the copied classes.

I would be open to clean it up as we think it's best. I do not see a point in maintaining compatibility with the original sources. If you have other such comments, please feel free to write them.

uce · 2018-05-23T15:14:58Z

I planned to upgrade netty and drop netty-router in one step of upgrading flink-shaded-netty. Do you think it should be split somehow?
No, if the ticket for upgrading covers that, we are all good.

👍 to merge. Thanks for taking care of this. Looking forward to finally have a recent Netty version.

This commit replaces netty-router dependency with our own version of it, which is simplified and adds guarantees about order of matching router patterns. This is a prerequisite for FLINK-3952. netty-router 1.10 is incompatible with Netty 4.1, while netty-router 2.2.0 brakes a compatibility in a way that we were unable to use it.

pnowojski · 2018-05-24T09:40:15Z

Thanks :) I have squashed commits and rebased the PR on latest master. Let's maybe wait for the final green travis before merging.

This commit replaces netty-router dependency with our own version of it, which is simplified and adds guarantees about order of matching router patterns. This is a prerequisite for FLINK-3952. netty-router 1.10 is incompatible with Netty 4.1, while netty-router 2.2.0 brakes a compatibility in a way that we were unable to use it. This closes apache#6031.

This commit replaces netty-router dependency with our own version of it, which is simplified and adds guarantees about order of matching router patterns. This is a prerequisite for FLINK-3952. netty-router 1.10 is incompatible with Netty 4.1, while netty-router 2.2.0 brakes a compatibility in a way that we were unable to use it. This closes #6031. Signed-off-by: Nico Kruber <nico@data-artisans.com>

pnowojski force-pushed the f9386 branch 2 times, most recently from 5f98447 to 018b6ef Compare May 17, 2018 10:28

uce approved these changes May 23, 2018

View reviewed changes

pnowojski added 2 commits May 24, 2018 11:39

[hotfix][tests] Report failure with error level instead of debug

7b3be7c

pnowojski force-pushed the f9386 branch from 0b9b98b to afaf1d5 Compare May 24, 2018 09:39

pnowojski mentioned this pull request May 24, 2018

[FLINK-3952][runtine] Upgrade to Netty 4.1 #6071

Closed

asfgit closed this in a5fa093 May 28, 2018

rmetzger added the component=<none> label Mar 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-9386] Embed netty router #6031

[FLINK-9386] Embed netty router #6031

pnowojski commented May 17, 2018

uce left a comment

uce May 23, 2018

uce May 23, 2018

uce May 23, 2018

uce May 23, 2018

uce May 23, 2018

pnowojski May 23, 2018

uce May 23, 2018

uce May 23, 2018

pnowojski May 23, 2018

uce May 23, 2018

uce May 23, 2018

uce May 23, 2018

uce May 23, 2018

uce May 23, 2018

pnowojski commented May 23, 2018

uce commented May 23, 2018

pnowojski commented May 24, 2018 •

edited

		CompletableFuture<FullHttpResponse> responseFuture;
		RouteResult result = routedRequest.getRouteResult();

Navigation Menu

[FLINK-9386] Embed netty router #6031

[FLINK-9386] Embed netty router #6031

Conversation

pnowojski commented May 17, 2018

Verifying this change

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

Documentation

uce left a 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

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

pnowojski commented May 23, 2018

uce commented May 23, 2018

pnowojski commented May 24, 2018 • edited

pnowojski commented May 24, 2018 •

edited