risdenk commented on a change in pull request #177: [WIP] KNOX-2095 - Adding in
DefaultDispatch code and tests to handle 504 errors
URL: https://github.com/apache/knox/pull/177#discussion_r346873542
##########
File path:
gateway-spi/src/main/java/org/apache/knox/gateway/dispatch/DefaultDispatch.java
##########
@@ -138,6 +144,10 @@ protected HttpResponse executeOutboundRequest(
HttpUriRequest outboundRequest )
}
}
auditor.audit( Action.DISPATCH, outboundRequest.getURI().toString(),
ResourceType.URI, ActionOutcome.SUCCESS, RES.responseStatus( statusCode ) );
+ } catch( SocketTimeoutException e ) {
+ //Set a 504 instead of throwing an IO exception in the event of a
socket timeout,
+ //as an IO exception forces a 500 to be displayed.
Review comment:
@shanyu you are arguing for changing the semantics of failover when this
change is all about just handling `SocketTimeoutException` as a 504 instead of
a 500.
Failover is a completely separate mechanism. If the topology is not
configured for failover then Knox won't failover. The `SocketTimeoutException`
handling shouldn't be any different in the case of a failover. It needs to tell
Knox that a failure happened. Not just assume it knows best and ignore the
possibility to failover.
Whether the current failover logic is correct or not is a completely
separate issue from handling `SocketTimeoutException`.
> I think letting client side know that the request caused timeout is very
important, instead of Letting them face a generic 500 error without knowing
what to do next.
As originally proposed, yes handling `SocketTimeoutException` and setting
the response to 504 makes sense, but it must do so in the framework that exists
for failover today.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
