flink-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sachingoel0101 <...@git.apache.org>
Subject [GitHub] flink pull request: [FLINK-2978][web-dashboard][webclient] Integra...
Date Sun, 22 Nov 2015 14:57:51 GMT
Github user sachingoel0101 commented on the pull request:

    https://github.com/apache/flink/pull/1338#issuecomment-158765098
  
    @rmetzger 
    1. I have introduced a separate directory for uploads, named randomly, and deleted as
part of the shut down hook. 
    2. I also tried closing the `URLClassLoader`, but it still doesn't allow for the Jars
to be deleted. I will look further into it and see if I can get it to work. It shouldn't be
a blocker however for merging this IMO. 
    
    @StephanEwen 
    1. I have removed the key to specify the upload directory, which takes care of naming
of the config entry. It now is just `jobmanager.web.submit.allow`
    2. What would be the best way to separate the passing of path and query parameters? Of
course, the most obvious choice is to change the method signature for `handleRequest` but
it appears too much of a change, just to allow query params. Since the interface is designed
by us, I think we can keep them together, and disallow any query param which overrides the
path param. Otherwise, if we do indeed change this, I suggest a construct `Parameters` with
fields `pathParams` and `queryParams`, with access method as `getParam(key, enum{PATH, QUERY})`
This will be a much cleaner solution.
    3. I have some concerns about error handling. There are four handlers: 
      a. `StaticFileServerHandler`: Handles exception events itself, by sending an `INTERNAL_SERVER_ERROR`
      b. `RuntimeMonitorHandler`: Handles all exceptions itself, with either a `NOT_FOUND`
or `INTERNAL_SERVER_ERROR` code.
      c. `HttpRequestHandler` [introduced in this PR]: Doesn't handler exceptions. But I'm
inclined towards sending an `INTERNAL_SERVER_ERROR` code for any exceptions here.
      d. `PipelineErrorHandler` [introduced in this PR]: If an exception caught event is fired
here, it can only happen because the netty threw an error [as reported by @gyfora in an instance].
There's really nothing to do here except sending an internal server error.
    What do you think?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message