[FLINK-5870] Handlers define REST URLs #3376
Conversation
uce
left a comment
uce
left a comment
There was a problem hiding this comment.
Really good addition with the tests. Just to verify: for the history server we take the returned paths and "expand" the variables:
/subtasks/:subtasknum =>
/subtasks/0
/subtasks/1
...
/subtasks/N
I like the idea of the RestUtils, but I think they decrease readability of the handlers. Would you mind removing it? :( The tests you added should catch all typos etc.
| this.httpsEnabled = httpsEnabled; | ||
| } | ||
|
|
||
| public abstract String[] getPaths(); |
There was a problem hiding this comment.
I know the other methods are also not commented well, but could you add comments here?
| // send an Access Denied message | ||
| JarAccessDeniedHandler jad = new JarAccessDeniedHandler(); | ||
| GET(router, jad); | ||
| POST(router, jad); |
There was a problem hiding this comment.
Why do we add the POST and DELETE here?
There was a problem hiding this comment.
Because there are Jar-related handlers that accept POST and DELETE requests. Having the JarAccessDeniedHandler reject these as well seemed more consistent.
There was a problem hiding this comment.
OK I thought that they are automatically rejected because we did not register a route for them (only for GET before)
There was a problem hiding this comment.
I think they got a 404 previously as unmatched paths are handled by the StaticFileServer.
| LOG.info("Web frontend listening at " + address + ':' + port); | ||
| } | ||
|
|
||
| private void GET(Router router, RequestHandler handler) { |
There was a problem hiding this comment.
Can we add a high level comment that says that we have these helpers in order to do the route path setup?
|
|
||
| @Override | ||
| public String[] getPaths() { | ||
| if (serveLogFile) { |
There was a problem hiding this comment.
I think the proper way to do this would be to have an abstract TaskManagerLogHandler and inherit it once for the logs and once for stdout.
There was a problem hiding this comment.
I think adding 2 classes for a single flag is overkill.
| } | ||
|
|
||
| @Override | ||
| public String[] getPaths() { |
There was a problem hiding this comment.
Just to double check: The handler is OK to handle both requests, right? Before we created two instances of the handler. I think this is actually a case where this works better than before.
There was a problem hiding this comment.
yes, the same handler can handle both requests. The handlers that we created before were identical.
| @Override | ||
| public String[] getPaths() { | ||
| return new String[]{CHECKPOINT_STATS_DETAILS_SUBTASKS_REST_PATH}; | ||
| } |
| */ | ||
| package org.apache.flink.runtime.webmonitor.utils; | ||
|
|
||
| public class RestUtils { |
There was a problem hiding this comment.
I see why you added this, but I'm again skeptical 😄 The problem I have is that it really decreases readability in my opinion in the handlers. I would actually vote to remove it and keep the complete paths in the handlers.
There was a problem hiding this comment.
The point of this class will be more apparent when the Archiver pattern is integrated. see zentol@83eff62
|
@uce I've update the PR. |
|
Thanks for addressing my comments. +1 to merge. Really looking forward to the next PRs with the history server. |
This closes apache#3376.
|
merging. |
This closes apache#3376.
3 participants
This PR allows
RequestHandlers to define the REST URLs under which they should be registered.For this purpose the following method was added to the
RequestHandlerinterface:String[] getPaths();Additionally, a utility class
RestUtilswas added that contains a number of often used REST URL components as constants (things like "job" or ":jobid"), and a utility method to concatenate these components into a REST URL (basically concat with "/"). The idea here is to prevent typos and such.Tests were added for every single handler to verify that the correct paths are returned. As a result if any URL should be changed, which isn't allowed since the REST API is considered stable, a test will now fail.