You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by EronWright <gi...@git.apache.org> on 2017/10/04 00:44:47 UTC
[GitHub] flink pull request #4765: [FLINK-7753] [flip-6] close REST channel on server...
GitHub user EronWright opened a pull request:
https://github.com/apache/flink/pull/4765
[FLINK-7753] [flip-6] close REST channel on server error
## What is the purpose of the change
Improve REST server handling of unexpected errors.
## Brief change log
- Close the connection when returning a 500 response.
- Obey rfc2616#section-14.10 which says that HTTP 1.1 responses should explicitly say `Connection: close` if keep-alive was requested but not agreed to.
## Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
- 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
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/EronWright/flink FLINK-7753
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/4765.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #4765
----
commit 1f9fef31323750cbaa8bd17fa7df93c7eb14a09f
Author: Wright, Eron <er...@emc.com>
Date: 2017-09-29T18:08:25Z
[FLINK-7753] [flip-6] close REST channel on server error, obey rfc2616#section-14.10
----
---
[GitHub] flink pull request #4765: [FLINK-7753] [flip-6] close REST channel on server...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/4765#discussion_r142666553
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/util/HandlerUtils.java ---
@@ -108,19 +108,25 @@ public static void sendErrorResponse(
* @param httpRequest originating http request
* @param message which should be sent
* @param statusCode of the message to send
+ * @param forceClose indicates whether to forcibly close the connection after the response is sent
*/
public static void sendResponse(
@Nonnull ChannelHandlerContext channelHandlerContext,
@Nonnull HttpRequest httpRequest,
@Nonnull String message,
- @Nonnull HttpResponseStatus statusCode) {
+ @Nonnull HttpResponseStatus statusCode,
+ boolean forceClose) {
--- End diff --
Would it make sense to not pass forceClose explicitly but to decide this based on `statusCode` and `isServerError` inside of this method?
---
[GitHub] flink pull request #4765: [FLINK-7753] [flip-6] close REST channel on server...
Posted by EronWright <gi...@git.apache.org>.
Github user EronWright commented on a diff in the pull request:
https://github.com/apache/flink/pull/4765#discussion_r142713741
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/util/HandlerUtils.java ---
@@ -108,19 +108,25 @@ public static void sendErrorResponse(
* @param httpRequest originating http request
* @param message which should be sent
* @param statusCode of the message to send
+ * @param forceClose indicates whether to forcibly close the connection after the response is sent
*/
public static void sendResponse(
@Nonnull ChannelHandlerContext channelHandlerContext,
@Nonnull HttpRequest httpRequest,
@Nonnull String message,
- @Nonnull HttpResponseStatus statusCode) {
+ @Nonnull HttpResponseStatus statusCode,
+ boolean forceClose) {
--- End diff --
I took this method to be the implementation method having maximum flexibility. Happy to change it.
---
[GitHub] flink pull request #4765: [FLINK-7753] [flip-6] close REST channel on server...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/4765#discussion_r142893839
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/util/HandlerUtils.java ---
@@ -108,19 +108,25 @@ public static void sendErrorResponse(
* @param httpRequest originating http request
* @param message which should be sent
* @param statusCode of the message to send
+ * @param forceClose indicates whether to forcibly close the connection after the response is sent
*/
public static void sendResponse(
@Nonnull ChannelHandlerContext channelHandlerContext,
@Nonnull HttpRequest httpRequest,
@Nonnull String message,
- @Nonnull HttpResponseStatus statusCode) {
+ @Nonnull HttpResponseStatus statusCode,
+ boolean forceClose) {
--- End diff --
I would prefer to keep the interface (also internal ones) as lean as possible. Past experience shows that this can otherwise lead to problems down the road (APIs grown too complicated, misuse). If need should arise, then we could change it accordingly.
---