Add socket timeout configuration in gRPC client and server #12845
Conversation
Codecov Report
@@ Coverage Diff @@
## release-0.990.0 #12845 +/- ##
==================================================
Coverage ? 60.02%
Complexity ? 659
==================================================
Files ? 2068
Lines ? 101681
Branches ? 12936
==================================================
Hits ? 61030
Misses ? 35477
Partials ? 5174
Continue to review full report at Codecov.
|
| } | ||
| long timeoutMillis = clientEndpointConfig.getIntField(CLIENT_EP_ENDPOINT_TIMEOUT); | ||
| if (timeoutMillis < 0 || !isInteger(timeoutMillis)) { | ||
| throw new BallerinaConnectorException("invalid idle timeout: " + timeoutMillis); |
There was a problem hiding this comment.
Instead of throwing a runtime exception what if we say that if it is <=0 then we are disabling the idleTimeout and if it is not an integer we log a warning and set it to Integer.MaxValue. This is because it would prevent the application crashing just because the user made a mistake of using large value (which is actually allowed in Ballerina) This is how we have done in WebSocket.
There was a problem hiding this comment.
Current gRPC implementation, we throw exception for any invalid user input. If we are going with default for invalid user input, we need to change all places where applied. I will review it make the change later. For now shall we keep this to maintain consistency.
There was a problem hiding this comment.
IMO it is best to avoid crashing the app as much as possible. +1 for doing it in a different PR.
tests/ballerina-integration-test/src/test/resources/grpc/clients/grpc_client_socket_timeout.bal
Show resolved
Hide resolved
tests/ballerina-integration-test/src/test/resources/grpc/clients/grpc_client_socket_timeout.bal
Outdated
Show resolved
Hide resolved
tests/ballerina-integration-test/src/test/resources/grpc/clients/grpc_client_socket_timeout.bal
Outdated
Show resolved
Hide resolved
...rina-integration-test/src/test/resources/grpc/grpcservices/14_grpc_client_socket_timeout.bal
Show resolved
Hide resolved
| @@ -185,7 +186,8 @@ public void sendMessage(Message message) { | |||
| try { | |||
| InputStream resp = method.streamRequest(message); | |||
| outboundMessage.sendMessage(resp); | |||
|
|
|||
| } catch (StatusRuntimeException ex) { | |||
There was a problem hiding this comment.
Why we need to catch this if we don't do anything? Since this is a RuntimeException shall we leave without handling it?
There was a problem hiding this comment.
Earlier for any exception we throw StatusRuntimeException and we need to avoid reconstructing exception if it is StatusRuntimeException.
Purpose
Resolves #11276