Github user pnowojski commented on a diff in the pull request:
https://github.com/apache/flink/pull/6031#discussion_r190272895
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/router/MethodlessRouter.java
---
@@ -0,0 +1,121 @@
+/*
+ * 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.router;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+
+/**
+ * This is adopted and simplified code from tv.cntt:netty-router library.
For more information check {@link Router}.
+ *
+ * <p>Router that doesn't contain information about HTTP request methods
and route
+ * matching orders.
+ */
+final class MethodlessRouter<T> {
+ 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<>();
--- End diff --
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.
---
