pkuwm commented on a change in pull request #1013:
URL: https://github.com/apache/helix/pull/1013#discussion_r428421156
##########
File path:
helix-rest/src/test/java/org/apache/helix/rest/client/TestCustomRestClient.java
##########
@@ -145,6 +148,35 @@ public void testPostRequestFormat() throws IOException {
Assert.assertEquals(json.get("data").asText(), "{}");
}
+ @Test
+ public void testGetInstanceStoppableCheckSmallTimeout() throws Exception {
+ // Reset the INSTANCE in CustomRestClientFactory to create new one with
new TimeOut
+ Field instance =
CustomRestClientFactory.class.getDeclaredField("INSTANCE");
+ instance.setAccessible(true);
+ instance.set(null, null);
+
+ // Set 1 ms to cause timeout for http requests
+ System.setProperty(RestSystemPropertyKeys.HTTP_TIMEOUT_MS, "1");
+ // a popular echo server that echos all the inputs
+ // TODO: add a mock rest server
+ final String echoServer = "http://httpbin.org/post";;
+ CustomRestClient _customRestClient = CustomRestClientFactory.get();
+ boolean timeoutExceptionHappened = false;
+ try {
+ _customRestClient.getInstanceStoppableCheck(echoServer,
Collections.emptyMap());
+ } catch (ConnectTimeoutException e) {
+ // Since the timeout is so small, we are expecting to get
ConnectTimeoutException
+ timeoutExceptionHappened = true;
+ }
+ // Reset the HTTP_REQUEST_TIMEOUT property back to the default value
+ System.setProperty(RestSystemPropertyKeys.HTTP_TIMEOUT_MS, "60000");
Review comment:
Why is this reset to a hard coded timeout? What if the timeout is
changed, you setting it here 60000 would break the original value. The correct
way should be to backup the original property and set it back here.
##########
File path:
helix-rest/src/main/java/org/apache/helix/rest/common/RestSystemPropertyKeys.java
##########
@@ -0,0 +1,6 @@
+package org.apache.helix.rest.common;
+
+public class RestSystemPropertyKeys {
+ // System property for request timeout
+ public static final String HTTP_TIMEOUT_MS = "http.timeout.ms";
Review comment:
Is this changed? Like I said in previous thread, The property name is
too generic. If this is set, it indicates that all http in helix should be
using the config. However this is not the case. Should be narrowed down to
helix rest or even stoppable.
##########
File path:
helix-rest/src/test/java/org/apache/helix/rest/client/TestCustomRestClient.java
##########
@@ -145,6 +148,35 @@ public void testPostRequestFormat() throws IOException {
Assert.assertEquals(json.get("data").asText(), "{}");
}
+ @Test
+ public void testGetInstanceStoppableCheckSmallTimeout() throws Exception {
+ // Reset the INSTANCE in CustomRestClientFactory to create new one with
new TimeOut
+ Field instance =
CustomRestClientFactory.class.getDeclaredField("INSTANCE");
+ instance.setAccessible(true);
+ instance.set(null, null);
+
+ // Set 1 ms to cause timeout for http requests
+ System.setProperty(RestSystemPropertyKeys.HTTP_TIMEOUT_MS, "1");
+ // a popular echo server that echos all the inputs
+ // TODO: add a mock rest server
+ final String echoServer = "http://httpbin.org/post";;
+ CustomRestClient _customRestClient = CustomRestClientFactory.get();
+ boolean timeoutExceptionHappened = false;
+ try {
+ _customRestClient.getInstanceStoppableCheck(echoServer,
Collections.emptyMap());
+ } catch (ConnectTimeoutException e) {
+ // Since the timeout is so small, we are expecting to get
ConnectTimeoutException
Review comment:
I am not sure if I am convinced by this. What if it is fast enough to
complete the request in 1 ms? It may not happen, but theoretically it could.
This is relying on possibility of timeout..
----------------------------------------------------------------
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
