You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/05/15 18:47:31 UTC
[GitHub] [helix] alirezazamani opened a new pull request #1013: Add timeout for stoppable post request
alirezazamani opened a new pull request #1013:
URL: https://github.com/apache/helix/pull/1013
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR title:
Fixes #1012
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
In this PR, http timeout has been added for the POST requests
which is mainly used for REST's stoppable check.
### Tests
- [x] The following tests are written for this issue:
TestCustomRestClient.testPostRequestSmallTimeout
- [x] The following is the result of the "mvn test" command on the appropriate module:
Helix-Core
```
[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR] TestStateTransitionTimeout.testStateTransitionTimeOut:170 expected:<true> but was:<false>
[ERROR] TestUpdateWorkflow.testUpdateRunningQueue:82 expected:<true> but was:<false>
[ERROR] TestWorkflowTermination.testWorkflowPausedTimeout:170->verifyWorkflowCleanup:257 expected:<true> but was:<false>
[INFO]
[ERROR] Tests run: 1146, Failures: 3, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:22 h
[INFO] Finished at: 2020-05-14T21:41:45-07:00
[INFO] ------------------------------------------------------------------------
```
The failed tests have passed when I ran them individually.
```
mvn test -Dtest="TestStateTransitionTimeout,TestUpdateWorkflow,TestWorkflowTermination"
[INFO] Tests run: 8, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 62.382 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 8, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:07 min
[INFO] Finished at: 2020-05-15T11:29:16-07:00
[INFO] ------------------------------------------------------------------------
```
Helix-REST:
```
[INFO] Tests run: 160, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 41.568 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 160, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 47.282 s
[INFO] Finished at: 2020-05-15T11:46:34-07:00
[INFO] ------------------------------------------------------------------------
```
### Commits
- [x] My commits all reference appropriate Apache Helix GitHub issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- [x] My diff has been formatted using helix-style.xml
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1013:
URL: https://github.com/apache/helix/pull/1013#discussion_r427587162
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/client/CustomRestClientFactory.java
##########
@@ -32,10 +36,13 @@
*/
public class CustomRestClientFactory {
private static final Logger LOG = LoggerFactory.getLogger(CustomRestClientFactory.class);
+ private static final int DEFAULT_HTTP_REQUEST_TIMEOUT = 60 * 1000;
Review comment:
Interesting. In that case, could we leave a comment? That way, the intention is clear, and as the writer of this PR, you can justify using an int.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1013:
URL: https://github.com/apache/helix/pull/1013#discussion_r427645656
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/client/TestCustomRestClient.java
##########
@@ -161,4 +179,35 @@ protected JsonNode getJsonObject(HttpResponse httpResponse) throws IOException {
return new ObjectMapper().readTree(_jsonResponse);
}
}
+
+ /**
+ * This MockCustomRestClientFactory is necessary to have for testing because once an INSTANCE is
+ * initialized in CustomRestClientFactory while running "mvn test" , it will no re-initialize the
+ * INSTANCE with new HelixProperty. Hence this class makes sure that new CustomRestClient will be
+ * created with the timeout set to the new value.
+ */
+ private static class MockCustomRestClientFactory extends CustomRestClientFactory {
Review comment:
Very good suggestion. Addressed. Thanks.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1013:
URL: https://github.com/apache/helix/pull/1013#discussion_r428423408
##########
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:
Should use try finally to recover the state. Otherwise if this test throws exception before recovering, later tests may be failing.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1013:
URL: https://github.com/apache/helix/pull/1013#discussion_r428777842
##########
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:
Removed this test.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1013:
URL: https://github.com/apache/helix/pull/1013#discussion_r427570174
##########
File path: helix-common/src/main/java/org/apache/helix/SystemPropertyKeys.java
##########
@@ -78,4 +78,7 @@
// System Property Metadata Store Directory Server endpoint key
public static final String MSDS_SERVER_ENDPOINT_KEY =
MetadataStoreRoutingConstants.MSDS_SERVER_ENDPOINT_KEY;
+
+ // System property for request timeout
+ public static final String HTTP_REQUEST_TIMEOUT = "http.request.timeout";
Review comment:
Done. Added a file called RestSystemPropertyKeys and added this key to that file.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1013:
URL: https://github.com/apache/helix/pull/1013#discussion_r428777658
##########
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:
Changed the test completely. It is using mock now.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1013:
URL: https://github.com/apache/helix/pull/1013#discussion_r427571161
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/client/TestCustomRestClient.java
##########
@@ -161,4 +179,35 @@ protected JsonNode getJsonObject(HttpResponse httpResponse) throws IOException {
return new ObjectMapper().readTree(_jsonResponse);
}
}
+
+ /**
+ * This MockCustomRestClientFactory is necessary to have for testing because once an INSTANCE is
+ * initialized in CustomRestClientFactory while running "mvn test" , it will no re-initialize the
+ * INSTANCE with new HelixProperty. Hence this class makes sure that new CustomRestClient will be
+ * created with the timeout set to the new value.
+ */
+ private static class MockCustomRestClientFactory extends CustomRestClientFactory {
+ private static final int DEFAULT_HTTP_REQUEST_TIMEOUT = 60 * 1000;
Review comment:
Please see my answer above. If you strongly feel we should do long, I can do it. But I think this way the code will be cleaner.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] dasahcc merged pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
dasahcc merged pull request #1013:
URL: https://github.com/apache/helix/pull/1013
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1013:
URL: https://github.com/apache/helix/pull/1013#discussion_r428777397
##########
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:
Changed it to REST_HTTP_TIMEOUT_MS.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1013:
URL: https://github.com/apache/helix/pull/1013#discussion_r427645855
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/client/CustomRestClientFactory.java
##########
@@ -32,10 +36,13 @@
*/
public class CustomRestClientFactory {
private static final Logger LOG = LoggerFactory.getLogger(CustomRestClientFactory.class);
+ private static final int DEFAULT_HTTP_REQUEST_TIMEOUT = 60 * 1000;
+ private static final int _httpRequestTimeout = HelixUtil.getSystemPropertyAsInt(
+ SystemPropertyKeys.HTTP_REQUEST_TIMEOUT, DEFAULT_HTTP_REQUEST_TIMEOUT);
private static CustomRestClient INSTANCE = null;
Review comment:
Yeah it can be in another PR. Thanks for mentioning it here.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1013:
URL: https://github.com/apache/helix/pull/1013#discussion_r428293447
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/client/TestCustomRestClient.java
##########
@@ -161,4 +179,35 @@ protected JsonNode getJsonObject(HttpResponse httpResponse) throws IOException {
return new ObjectMapper().readTree(_jsonResponse);
}
}
+
+ /**
+ * This MockCustomRestClientFactory is necessary to have for testing because once an INSTANCE is
+ * initialized in CustomRestClientFactory while running "mvn test" , it will no re-initialize the
+ * INSTANCE with new HelixProperty. Hence this class makes sure that new CustomRestClient will be
+ * created with the timeout set to the new value.
+ */
+ private static class MockCustomRestClientFactory extends CustomRestClientFactory {
+ private static final int DEFAULT_HTTP_REQUEST_TIMEOUT = 60 * 1000;
Review comment:
I added comments about it. I removed the default constant and directly uses the value in getSystemPropertyAsInt to avoid creating new constants. I found some other default values in for http request for MSDS related code. However, I am not sure if that is a good idea to have one default for everything. Anyway, please have a look and see if this makes sense.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1013:
URL: https://github.com/apache/helix/pull/1013#discussion_r427588117
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/client/TestCustomRestClient.java
##########
@@ -161,4 +179,35 @@ protected JsonNode getJsonObject(HttpResponse httpResponse) throws IOException {
return new ObjectMapper().readTree(_jsonResponse);
}
}
+
+ /**
+ * This MockCustomRestClientFactory is necessary to have for testing because once an INSTANCE is
+ * initialized in CustomRestClientFactory while running "mvn test" , it will no re-initialize the
+ * INSTANCE with new HelixProperty. Hence this class makes sure that new CustomRestClient will be
+ * created with the timeout set to the new value.
+ */
+ private static class MockCustomRestClientFactory extends CustomRestClientFactory {
+ private static final int DEFAULT_HTTP_REQUEST_TIMEOUT = 60 * 1000;
Review comment:
in that case, using an int is fine. But can we avoid creating yet another constant here? I believe we already define it elsewhere.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1013:
URL: https://github.com/apache/helix/pull/1013#discussion_r428298605
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/client/TestCustomRestClient.java
##########
@@ -145,6 +150,19 @@ public void testPostRequestFormat() throws IOException {
Assert.assertEquals(json.get("data").asText(), "{}");
}
+ @Test(expectedExceptions = ConnectTimeoutException.class)
+ public void testPostRequestSmallTimeout() throws IOException {
+ // Set 1 ms to cause timeout for requests
+ System.setProperty(SystemPropertyKeys.HTTP_REQUEST_TIMEOUT, "1");
+ // a popular echo server that echos all the inputs
+ final String echoServer = "http://httpbin.org/post";
Review comment:
I just realize that 127.0.0.0 gets timeout exception in mac only and not the Linux. So I changed it back to the external source for now. I added todo for this series of test to add mock rest server sometime in the future.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1013:
URL: https://github.com/apache/helix/pull/1013#discussion_r426227730
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/client/CustomRestClientImpl.java
##########
@@ -130,6 +131,11 @@ protected HttpResponse post(String url, Map<String, String> payloads) throws IOE
LOG.info("Executing request: {}, headers: {}, entity: {}", postRequest.getRequestLine(),
postRequest.getAllHeaders(), postRequest.getEntity());
return _httpClient.execute(postRequest);
+ } catch (ConnectTimeoutException e) {
+ LOG.error(
+ "Failed to perform customized health check due to ConnectTimeoutException for endpoint {}.",
+ url, e);
+ throw e;
Review comment:
What's the purpose of logging and throwing exception?
By checking the calling stack, I don't think it is necessary. `ConnectTimeoutException` also extends `IOException` which is already caught. The upper method `getHealthStatusFromRest` catches this exception and logs. If you log and throw here, there would be two error messages which are confusing and redundant. Eg, in the test you write, we would see the error log in our test output if we set logging level to print errors. And it is quite confusing.
##########
File path: helix-common/src/main/java/org/apache/helix/SystemPropertyKeys.java
##########
@@ -78,4 +78,7 @@
// System Property Metadata Store Directory Server endpoint key
public static final String MSDS_SERVER_ENDPOINT_KEY =
MetadataStoreRoutingConstants.MSDS_SERVER_ENDPOINT_KEY;
+
+ // System property for request timeout
+ public static final String HTTP_REQUEST_TIMEOUT = "http.request.timeout";
Review comment:
+1. There are other places like MSDS that also have timeout config. This should be narrow down to helix rest or even stoppable.
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/client/CustomRestClientFactory.java
##########
@@ -32,10 +36,13 @@
*/
public class CustomRestClientFactory {
private static final Logger LOG = LoggerFactory.getLogger(CustomRestClientFactory.class);
+ private static final int DEFAULT_HTTP_REQUEST_TIMEOUT = 60 * 1000;
+ private static final int _httpRequestTimeout = HelixUtil.getSystemPropertyAsInt(
+ SystemPropertyKeys.HTTP_REQUEST_TIMEOUT, DEFAULT_HTTP_REQUEST_TIMEOUT);
private static CustomRestClient INSTANCE = null;
Review comment:
It seems we should also change this to make it `volatile` so it is safe for double lock checking. But we should do it in another PR. Reference: https://github.com/apache/helix/issues/1014
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/client/CustomRestClientFactory.java
##########
@@ -32,10 +36,13 @@
*/
public class CustomRestClientFactory {
private static final Logger LOG = LoggerFactory.getLogger(CustomRestClientFactory.class);
+ private static final int DEFAULT_HTTP_REQUEST_TIMEOUT = 60 * 1000;
+ private static final int _httpRequestTimeout = HelixUtil.getSystemPropertyAsInt(
Review comment:
Could combine `DEFAULT_HTTP_REQUEST_TIMEOUT` and this, right? And also this var is static final, would be better to name it a constant?
Back from your test, if we don't make this a constant but a local var in get(), we could avoid the `MockCustomRestClientFactory` by resetting the instance using reflection.
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/client/TestCustomRestClient.java
##########
@@ -145,6 +150,19 @@ public void testPostRequestFormat() throws IOException {
Assert.assertEquals(json.get("data").asText(), "{}");
}
+ @Test(expectedExceptions = ConnectTimeoutException.class)
+ public void testPostRequestSmallTimeout() throws IOException {
+ // Set 1 ms to cause timeout for requests
+ System.setProperty(SystemPropertyKeys.HTTP_REQUEST_TIMEOUT, "1");
+ // a popular echo server that echos all the inputs
+ final String echoServer = "http://httpbin.org/post";
+ CustomRestClient _customRestClient = MockCustomRestClientFactory.get();
+ _customRestClient.getInstanceStoppableCheck(echoServer, Collections.emptyMap());
Review comment:
This is unnecessary as the exception is thrown from `post()`. I suggest using `try...catch` so it is easier to know which code is tested. With `expectedExceptions`, it is not easy to know which line of code is expected to throw the exception as this method has multiple lines and API calls. If calling `_customRestClient.getInstanceStoppableCheck`, it is more like an integration test not a unit test. And as you state in the method name `testPostRequestSmallTimeout`, it is supposed to be testing the `post()`, right?
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/client/TestCustomRestClient.java
##########
@@ -161,4 +179,35 @@ protected JsonNode getJsonObject(HttpResponse httpResponse) throws IOException {
return new ObjectMapper().readTree(_jsonResponse);
}
}
+
+ /**
+ * This MockCustomRestClientFactory is necessary to have for testing because once an INSTANCE is
+ * initialized in CustomRestClientFactory while running "mvn test" , it will no re-initialize the
+ * INSTANCE with new HelixProperty. Hence this class makes sure that new CustomRestClient will be
+ * created with the timeout set to the new value.
+ */
+ private static class MockCustomRestClientFactory extends CustomRestClientFactory {
Review comment:
This mock class implements the almost the same logic of the parent class. The cons are:
- duplicate code
- difficult to maintain. if the parent class changes its logic, it is not easy to find this mock class and change this logic. If this child class is not changed, the test is actually testing the child class, not the logic in parent class.
There is a trick to avoid this duplicate and unnecessary mock class. We could use Java reflection to change the class's internal state, so the singleton would be re-initialized. And to be able to make the new instance use the new system property, the system property variable needs to be in a local one in `get()`.
```
// reset the singleton instance.
Field instance = Parent.class.getDeclaredField("INSTANCE");
instance.setAccessible(true);
instance.set(null, null);
```
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/client/TestCustomRestClient.java
##########
@@ -145,6 +150,19 @@ public void testPostRequestFormat() throws IOException {
Assert.assertEquals(json.get("data").asText(), "{}");
}
+ @Test(expectedExceptions = ConnectTimeoutException.class)
+ public void testPostRequestSmallTimeout() throws IOException {
+ // Set 1 ms to cause timeout for requests
+ System.setProperty(SystemPropertyKeys.HTTP_REQUEST_TIMEOUT, "1");
+ // a popular echo server that echos all the inputs
+ final String echoServer = "http://httpbin.org/post";
Review comment:
I don't think it is a good idea to connect to an external host/service for testing. What if my testing machine doesn't have network? Are we still going to get the expected exception? The answer is no. Instead, something else like `UnknownHostException` would be thrown. And this test does not achieve the purpose.
In a unit test, and in this case, I would not want to actually connect to a real REST server. Connecting to a real server is technically more an integration test. Though we have a local server started in `AbstractTestClass`, I would suggest mocking the timeout scenario for this unit test instead of connecting to a real server.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1013:
URL: https://github.com/apache/helix/pull/1013#discussion_r427570393
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/client/CustomRestClientFactory.java
##########
@@ -32,10 +36,13 @@
*/
public class CustomRestClientFactory {
private static final Logger LOG = LoggerFactory.getLogger(CustomRestClientFactory.class);
+ private static final int DEFAULT_HTTP_REQUEST_TIMEOUT = 60 * 1000;
Review comment:
I noticed that as well. But since the setSocketTimeout and setConnectionRequestTimeout methods are getting int as input, I prefer to keep them as int. If not we need to have additional type casting.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on pull request #1013:
URL: https://github.com/apache/helix/pull/1013#issuecomment-632228685
This PR is ready to be merged, approved by @dasahcc
Final commit message:
Add timeout for stoppable post request
In this commit, a http timeout has been added for the POST requests
which is mainly used for REST's stoppable check.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1013:
URL: https://github.com/apache/helix/pull/1013#discussion_r426055460
##########
File path: helix-common/src/main/java/org/apache/helix/SystemPropertyKeys.java
##########
@@ -78,4 +78,7 @@
// System Property Metadata Store Directory Server endpoint key
public static final String MSDS_SERVER_ENDPOINT_KEY =
MetadataStoreRoutingConstants.MSDS_SERVER_ENDPOINT_KEY;
+
+ // System property for request timeout
+ public static final String HTTP_REQUEST_TIMEOUT = "http.request.timeout";
Review comment:
Is this strictly for Helix REST? In that case, should we try to put this in a PropertyKey constant file in the helix-rest module?
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/client/CustomRestClientFactory.java
##########
@@ -32,10 +36,13 @@
*/
public class CustomRestClientFactory {
private static final Logger LOG = LoggerFactory.getLogger(CustomRestClientFactory.class);
+ private static final int DEFAULT_HTTP_REQUEST_TIMEOUT = 60 * 1000;
Review comment:
For timeout values, we usually use `long` type.
`60 * 1000L;`
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/client/TestCustomRestClient.java
##########
@@ -145,6 +150,19 @@ public void testPostRequestFormat() throws IOException {
Assert.assertEquals(json.get("data").asText(), "{}");
}
+ @Test(expectedExceptions = ConnectTimeoutException.class)
+ public void testPostRequestSmallTimeout() throws IOException {
+ // Set 1 ms to cause timeout for requests
+ System.setProperty(SystemPropertyKeys.HTTP_REQUEST_TIMEOUT, "1");
+ // a popular echo server that echos all the inputs
+ final String echoServer = "http://httpbin.org/post";
+ CustomRestClient _customRestClient = MockCustomRestClientFactory.get();
+ _customRestClient.getInstanceStoppableCheck(echoServer, Collections.emptyMap());
+
+ // Reset the HTTP_REQUEST_TIMEOUT property back to its original value
+ System.setProperty(SystemPropertyKeys.HTTP_REQUEST_TIMEOUT, "60000");
Review comment:
:"back to its original value" -> where is the original value set?
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/client/TestCustomRestClient.java
##########
@@ -161,4 +179,35 @@ protected JsonNode getJsonObject(HttpResponse httpResponse) throws IOException {
return new ObjectMapper().readTree(_jsonResponse);
}
}
+
+ /**
+ * This MockCustomRestClientFactory is necessary to have for testing because once an INSTANCE is
+ * initialized in CustomRestClientFactory while running "mvn test" , it will no re-initialize the
Review comment:
Please fix typos.
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/client/TestCustomRestClient.java
##########
@@ -161,4 +179,35 @@ protected JsonNode getJsonObject(HttpResponse httpResponse) throws IOException {
return new ObjectMapper().readTree(_jsonResponse);
}
}
+
+ /**
+ * This MockCustomRestClientFactory is necessary to have for testing because once an INSTANCE is
+ * initialized in CustomRestClientFactory while running "mvn test" , it will no re-initialize the
+ * INSTANCE with new HelixProperty. Hence this class makes sure that new CustomRestClient will be
+ * created with the timeout set to the new value.
+ */
+ private static class MockCustomRestClientFactory extends CustomRestClientFactory {
+ private static final int DEFAULT_HTTP_REQUEST_TIMEOUT = 60 * 1000;
Review comment:
Use long
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/client/CustomRestClientFactory.java
##########
@@ -44,14 +51,17 @@ public static CustomRestClient get() {
if (INSTANCE == null) {
try {
HttpClient httpClient;
+ RequestConfig config = RequestConfig.custom().setConnectTimeout(_httpRequestTimeout)
+ .setConnectionRequestTimeout(_httpRequestTimeout)
+ .setSocketTimeout(_httpRequestTimeout).build();
Review comment:
Do we want to set the socket and connect timeout as well? Those two seem unrelated to the problem you're solving. I am not saying we shouldn't necessarily, but we should understand the implications of setting such configs.
Also it would be good to add a line of comment describing what you're doing here.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1013:
URL: https://github.com/apache/helix/pull/1013#discussion_r428293597
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/client/CustomRestClientFactory.java
##########
@@ -32,10 +36,13 @@
*/
public class CustomRestClientFactory {
private static final Logger LOG = LoggerFactory.getLogger(CustomRestClientFactory.class);
+ private static final int DEFAULT_HTTP_REQUEST_TIMEOUT = 60 * 1000;
Review comment:
Sure. Added a comments.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1013:
URL: https://github.com/apache/helix/pull/1013#discussion_r428331977
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/client/CustomRestClientFactory.java
##########
@@ -32,26 +37,32 @@
*/
public class CustomRestClientFactory {
private static final Logger LOG = LoggerFactory.getLogger(CustomRestClientFactory.class);
-
private static CustomRestClient INSTANCE = null;
- private CustomRestClientFactory() {
+ protected CustomRestClientFactory() {
}
public static CustomRestClient get() {
if (INSTANCE == null) {
synchronized (CustomRestClientFactory.class) {
if (INSTANCE == null) {
try {
+ // Here int has been used for timeout value because setConnectTimeout,
+ // setConnectionRequestTimeout and setSocketTimeout are getting int as input
+ final int httpRequestTimeout =
+ HelixUtil.getSystemPropertyAsInt(RestSystemPropertyKeys.HTTP_TIMEOUT_MS, 60 * 1000);
Review comment:
Sure. Moved to constant file.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1013:
URL: https://github.com/apache/helix/pull/1013#discussion_r428317429
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/client/CustomRestClientFactory.java
##########
@@ -32,26 +37,32 @@
*/
public class CustomRestClientFactory {
private static final Logger LOG = LoggerFactory.getLogger(CustomRestClientFactory.class);
-
private static CustomRestClient INSTANCE = null;
- private CustomRestClientFactory() {
+ protected CustomRestClientFactory() {
}
public static CustomRestClient get() {
if (INSTANCE == null) {
synchronized (CustomRestClientFactory.class) {
if (INSTANCE == null) {
try {
+ // Here int has been used for timeout value because setConnectTimeout,
+ // setConnectionRequestTimeout and setSocketTimeout are getting int as input
+ final int httpRequestTimeout =
+ HelixUtil.getSystemPropertyAsInt(RestSystemPropertyKeys.HTTP_TIMEOUT_MS, 60 * 1000);
Review comment:
Let's put this number to some constant file or at least to be a variable
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1013:
URL: https://github.com/apache/helix/pull/1013#discussion_r428290917
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/client/TestCustomRestClient.java
##########
@@ -145,6 +150,19 @@ public void testPostRequestFormat() throws IOException {
Assert.assertEquals(json.get("data").asText(), "{}");
}
+ @Test(expectedExceptions = ConnectTimeoutException.class)
+ public void testPostRequestSmallTimeout() throws IOException {
+ // Set 1 ms to cause timeout for requests
+ System.setProperty(SystemPropertyKeys.HTTP_REQUEST_TIMEOUT, "1");
+ // a popular echo server that echos all the inputs
+ final String echoServer = "http://httpbin.org/post";
Review comment:
I changed the real server. Now, I am using non-routable internal address which causes the ConnectTimeoutException for http requests.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1013:
URL: https://github.com/apache/helix/pull/1013#discussion_r428928123
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/client/TestCustomRestClient.java
##########
@@ -145,6 +146,28 @@ public void testPostRequestFormat() throws IOException {
Assert.assertEquals(json.get("data").asText(), "{}");
}
+ @Test
+ public void testGetPartitionStoppableCheck_when_timeout() throws IOException {
Review comment:
This mix of styling (snake_case and camelCase) isn't java style
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/client/CustomRestClientImpl.java
##########
@@ -33,6 +33,7 @@
import org.apache.http.client.ClientProtocolException;
import org.apache.http.client.HttpClient;
import org.apache.http.client.methods.HttpPost;
+import org.apache.http.conn.ConnectTimeoutException;
Review comment:
Please remove this unused import
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1013:
URL: https://github.com/apache/helix/pull/1013#discussion_r427645571
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/client/CustomRestClientImpl.java
##########
@@ -130,6 +131,11 @@ protected HttpResponse post(String url, Map<String, String> payloads) throws IOE
LOG.info("Executing request: {}, headers: {}, entity: {}", postRequest.getRequestLine(),
postRequest.getAllHeaders(), postRequest.getEntity());
return _httpClient.execute(postRequest);
+ } catch (ConnectTimeoutException e) {
+ LOG.error(
+ "Failed to perform customized health check due to ConnectTimeoutException for endpoint {}.",
+ url, e);
+ throw e;
Review comment:
Removed the log and exception as it will be catched (IOException) by upper methods. Thanks.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1013:
URL: https://github.com/apache/helix/pull/1013#discussion_r427645219
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/client/CustomRestClientFactory.java
##########
@@ -32,10 +36,13 @@
*/
public class CustomRestClientFactory {
private static final Logger LOG = LoggerFactory.getLogger(CustomRestClientFactory.class);
+ private static final int DEFAULT_HTTP_REQUEST_TIMEOUT = 60 * 1000;
+ private static final int _httpRequestTimeout = HelixUtil.getSystemPropertyAsInt(
Review comment:
I combined the Default and getSystemProperty. Thanks.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1013:
URL: https://github.com/apache/helix/pull/1013#discussion_r427570458
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/client/CustomRestClientFactory.java
##########
@@ -44,14 +51,17 @@ public static CustomRestClient get() {
if (INSTANCE == null) {
try {
HttpClient httpClient;
+ RequestConfig config = RequestConfig.custom().setConnectTimeout(_httpRequestTimeout)
+ .setConnectionRequestTimeout(_httpRequestTimeout)
+ .setSocketTimeout(_httpRequestTimeout).build();
Review comment:
I think we prefer to have a timeout for any reason and not be blocked to query SN. That is why I used both of them as same value. Also I noticed that we do same for other classes such as HttpRoutingDataReader. I do not have strong preferences about this. I feel we should reduce the amount of configs as much as possible.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1013:
URL: https://github.com/apache/helix/pull/1013#discussion_r427646423
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/client/TestCustomRestClient.java
##########
@@ -161,4 +179,35 @@ protected JsonNode getJsonObject(HttpResponse httpResponse) throws IOException {
return new ObjectMapper().readTree(_jsonResponse);
}
}
+
+ /**
+ * This MockCustomRestClientFactory is necessary to have for testing because once an INSTANCE is
+ * initialized in CustomRestClientFactory while running "mvn test" , it will no re-initialize the
Review comment:
Removed. Thanks. I realize this class can be removed and not needed.
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/client/TestCustomRestClient.java
##########
@@ -145,6 +150,19 @@ public void testPostRequestFormat() throws IOException {
Assert.assertEquals(json.get("data").asText(), "{}");
}
+ @Test(expectedExceptions = ConnectTimeoutException.class)
+ public void testPostRequestSmallTimeout() throws IOException {
+ // Set 1 ms to cause timeout for requests
+ System.setProperty(SystemPropertyKeys.HTTP_REQUEST_TIMEOUT, "1");
+ // a popular echo server that echos all the inputs
+ final String echoServer = "http://httpbin.org/post";
+ CustomRestClient _customRestClient = MockCustomRestClientFactory.get();
+ _customRestClient.getInstanceStoppableCheck(echoServer, Collections.emptyMap());
+
+ // Reset the HTTP_REQUEST_TIMEOUT property back to its original value
+ System.setProperty(SystemPropertyKeys.HTTP_REQUEST_TIMEOUT, "60000");
Review comment:
Fixed. Thanks.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #1013: Add timeout for stoppable post request
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1013:
URL: https://github.com/apache/helix/pull/1013#discussion_r428153466
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/client/TestCustomRestClient.java
##########
@@ -145,6 +150,19 @@ public void testPostRequestFormat() throws IOException {
Assert.assertEquals(json.get("data").asText(), "{}");
}
+ @Test(expectedExceptions = ConnectTimeoutException.class)
+ public void testPostRequestSmallTimeout() throws IOException {
+ // Set 1 ms to cause timeout for requests
+ System.setProperty(SystemPropertyKeys.HTTP_REQUEST_TIMEOUT, "1");
+ // a popular echo server that echos all the inputs
+ final String echoServer = "http://httpbin.org/post";
+ CustomRestClient _customRestClient = MockCustomRestClientFactory.get();
+ _customRestClient.getInstanceStoppableCheck(echoServer, Collections.emptyMap());
Review comment:
Basically wanted to check getInstanceStoppableCheck. I will change the Test's name. And I will change it to try catch.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org