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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
