javeme commented on code in PR #133:
URL:
https://github.com/apache/incubator-hugegraph-commons/pull/133#discussion_r1390429539
##########
hugegraph-common/src/main/java/org/apache/hugegraph/rest/RestClientConfig.java:
##########
@@ -24,15 +24,16 @@
@Builder
@Getter
@Setter
-public class OkHttpConfig {
+public class RestClientConfig {
private String user;
private String password;
private String token;
private Integer timeout;
- private Integer maxTotal;
- private Integer maxPerRoute;
- private Integer idleTime;
+ private Integer maxConns;
+ private Integer maxConnsPerRoute;
+ private Integer idleTime = 5;
Review Comment:
its unit is in seconds? we expect a default value > 10s like 30s, but I
think we don't need to set a default value here.
##########
hugegraph-common/src/test/java/org/apache/hugegraph/unit/rest/RestClientTest.java:
##########
@@ -84,7 +84,7 @@ public void testPost() {
@Test
// TODO: How to verify it?
- public void testPostWithMaxTotalAndPerRoute() {
+ public void testPostWithmaxConnsAndPerRoute() {
Review Comment:
max => Max
##########
hugegraph-common/src/main/java/org/apache/hugegraph/rest/HttpHeadersConstant.java:
##########
@@ -18,9 +18,17 @@
package org.apache.hugegraph.rest;
public class HttpHeadersConstant {
+
public static final String CONTENT_TYPE = "Content-Type";
+
public static final String CONTENT_ENCODING = "Content-Encoding";
+
public static final String AUTHORIZATION = "Authorization";
+
public static final String APPLICATION_JSON = "application/json";
+ public static final String BEARER = "Bearer";
+
+ public static final String SPACE = " ";
Review Comment:
we can define `BEARER_PREFIX = "Bearer "`
##########
hugegraph-common/src/main/java/org/apache/hugegraph/rest/AbstractRestClient.java:
##########
@@ -60,92 +60,92 @@ public abstract class AbstractRestClient implements
RestClient {
private final Request.Builder requestBuilder;
public AbstractRestClient(String url, int timeout) {
- this(url, OkHttpConfig.builder()
- .timeout(timeout)
- .build());
+ this(url, RestClientConfig.builder().timeout(timeout).build());
}
public AbstractRestClient(String url, String user, String password,
- Integer timeout) {
- this(url, OkHttpConfig.builder()
- .user(user).password(password)
- .timeout(timeout)
- .build());
+ int timeout) {
Review Comment:
seems one line is ok
##########
hugegraph-common/src/main/java/org/apache/hugegraph/rest/RestClientConfig.java:
##########
@@ -24,15 +24,16 @@
@Builder
@Getter
@Setter
-public class OkHttpConfig {
+public class RestClientConfig {
private String user;
private String password;
private String token;
private Integer timeout;
- private Integer maxTotal;
- private Integer maxPerRoute;
- private Integer idleTime;
+ private Integer maxConns;
+ private Integer maxConnsPerRoute;
+ private Integer idleTime = 5;
+ private Integer maxIdleConnections = 5;
Review Comment:
like maxConns, can we also keep maxIdleConns style
##########
hugegraph-common/src/main/java/org/apache/hugegraph/rest/AbstractRestClient.java:
##########
@@ -60,92 +60,92 @@ public abstract class AbstractRestClient implements
RestClient {
private final Request.Builder requestBuilder;
public AbstractRestClient(String url, int timeout) {
- this(url, OkHttpConfig.builder()
- .timeout(timeout)
- .build());
+ this(url, RestClientConfig.builder().timeout(timeout).build());
}
public AbstractRestClient(String url, String user, String password,
- Integer timeout) {
- this(url, OkHttpConfig.builder()
- .user(user).password(password)
- .timeout(timeout)
- .build());
+ int timeout) {
+ this(url, RestClientConfig.builder()
+ .user(user)
+ .password(password)
+ .timeout(timeout)
+ .build());
}
public AbstractRestClient(String url, int timeout,
- int maxTotal, int maxPerRoute) {
- this(url, null, null, timeout, maxTotal, maxPerRoute);
+ int maxConns, int maxConnsPerRoute) {
+ this(url, null, null, timeout, maxConns, maxConnsPerRoute);
}
public AbstractRestClient(String url, int timeout, int idleTime,
- int maxTotal, int maxPerRoute) {
- this(url, OkHttpConfig.builder()
- .idleTime(idleTime)
- .timeout(timeout)
- .maxTotal(maxTotal)
- .maxPerRoute(maxPerRoute)
- .build());
+ int maxConns, int maxConnsPerRoute) {
+ this(url, RestClientConfig.builder()
+ .idleTime(idleTime)
+ .timeout(timeout)
+ .maxConns(maxConns)
+ .maxConnsPerRoute(maxConnsPerRoute)
+ .build());
}
public AbstractRestClient(String url, String user, String password,
- int timeout, int maxTotal, int maxPerRoute) {
- this(url, OkHttpConfig.builder()
- .user(user).password(password)
- .timeout(timeout)
- .maxTotal(maxTotal)
- .maxPerRoute(maxPerRoute)
- .build());
+ int timeout, int maxConns, int maxConnsPerRoute)
{
+ this(url, RestClientConfig.builder()
+ .user(user)
+ .password(password)
+ .timeout(timeout)
+ .maxConns(maxConns)
+ .maxConnsPerRoute(maxConnsPerRoute)
+ .build());
}
public AbstractRestClient(String url, String user, String password,
- int timeout, int maxTotal, int maxPerRoute,
+ int timeout, int maxConns, int maxConnsPerRoute,
String trustStoreFile,
String trustStorePassword) {
- this(url, OkHttpConfig.builder()
- .user(user).password(password)
- .timeout(timeout)
- .maxTotal(maxTotal)
- .maxPerRoute(maxPerRoute)
- .trustStoreFile(trustStoreFile)
- .trustStorePassword(trustStorePassword)
- .build());
- }
-
- public AbstractRestClient(String url, String token, Integer timeout) {
- this(url, OkHttpConfig.builder()
- .token(token)
- .timeout(timeout)
- .build());
- }
-
- public AbstractRestClient(String url, String token, Integer timeout,
- Integer maxTotal, Integer maxPerRoute) {
- this(url, OkHttpConfig.builder()
- .token(token)
- .timeout(timeout)
- .maxTotal(maxTotal)
- .maxPerRoute(maxPerRoute)
- .build());
- }
-
- public AbstractRestClient(String url, String token, Integer timeout,
- Integer maxTotal, Integer maxPerRoute,
+ this(url, RestClientConfig.builder()
+ .user(user).password(password)
+ .timeout(timeout)
+ .maxConns(maxConns)
+ .maxConnsPerRoute(maxConnsPerRoute)
+ .trustStoreFile(trustStoreFile)
+ .trustStorePassword(trustStorePassword)
+ .build());
+ }
+
+ public AbstractRestClient(String url, String token, int timeout) {
Review Comment:
prefer to move this method to line 73
##########
hugegraph-common/src/main/java/org/apache/hugegraph/rest/AbstractRestClient.java:
##########
@@ -60,92 +60,92 @@ public abstract class AbstractRestClient implements
RestClient {
private final Request.Builder requestBuilder;
public AbstractRestClient(String url, int timeout) {
- this(url, OkHttpConfig.builder()
- .timeout(timeout)
- .build());
+ this(url, RestClientConfig.builder().timeout(timeout).build());
}
public AbstractRestClient(String url, String user, String password,
- Integer timeout) {
- this(url, OkHttpConfig.builder()
- .user(user).password(password)
- .timeout(timeout)
- .build());
+ int timeout) {
+ this(url, RestClientConfig.builder()
+ .user(user)
+ .password(password)
+ .timeout(timeout)
+ .build());
}
public AbstractRestClient(String url, int timeout,
- int maxTotal, int maxPerRoute) {
- this(url, null, null, timeout, maxTotal, maxPerRoute);
+ int maxConns, int maxConnsPerRoute) {
+ this(url, null, null, timeout, maxConns, maxConnsPerRoute);
}
public AbstractRestClient(String url, int timeout, int idleTime,
- int maxTotal, int maxPerRoute) {
- this(url, OkHttpConfig.builder()
- .idleTime(idleTime)
- .timeout(timeout)
- .maxTotal(maxTotal)
- .maxPerRoute(maxPerRoute)
- .build());
+ int maxConns, int maxConnsPerRoute) {
+ this(url, RestClientConfig.builder()
+ .idleTime(idleTime)
+ .timeout(timeout)
+ .maxConns(maxConns)
+ .maxConnsPerRoute(maxConnsPerRoute)
+ .build());
}
public AbstractRestClient(String url, String user, String password,
- int timeout, int maxTotal, int maxPerRoute) {
- this(url, OkHttpConfig.builder()
- .user(user).password(password)
- .timeout(timeout)
- .maxTotal(maxTotal)
- .maxPerRoute(maxPerRoute)
- .build());
+ int timeout, int maxConns, int maxConnsPerRoute)
{
Review Comment:
it's too many constructors, we can remove this method
##########
hugegraph-common/src/main/java/org/apache/hugegraph/rest/AbstractRestClient.java:
##########
@@ -60,92 +60,92 @@ public abstract class AbstractRestClient implements
RestClient {
private final Request.Builder requestBuilder;
public AbstractRestClient(String url, int timeout) {
- this(url, OkHttpConfig.builder()
- .timeout(timeout)
- .build());
+ this(url, RestClientConfig.builder().timeout(timeout).build());
}
public AbstractRestClient(String url, String user, String password,
- Integer timeout) {
- this(url, OkHttpConfig.builder()
- .user(user).password(password)
- .timeout(timeout)
- .build());
+ int timeout) {
+ this(url, RestClientConfig.builder()
+ .user(user)
+ .password(password)
+ .timeout(timeout)
+ .build());
}
public AbstractRestClient(String url, int timeout,
- int maxTotal, int maxPerRoute) {
- this(url, null, null, timeout, maxTotal, maxPerRoute);
+ int maxConns, int maxConnsPerRoute) {
+ this(url, null, null, timeout, maxConns, maxConnsPerRoute);
}
public AbstractRestClient(String url, int timeout, int idleTime,
- int maxTotal, int maxPerRoute) {
- this(url, OkHttpConfig.builder()
- .idleTime(idleTime)
- .timeout(timeout)
- .maxTotal(maxTotal)
- .maxPerRoute(maxPerRoute)
- .build());
+ int maxConns, int maxConnsPerRoute) {
+ this(url, RestClientConfig.builder()
+ .idleTime(idleTime)
+ .timeout(timeout)
+ .maxConns(maxConns)
+ .maxConnsPerRoute(maxConnsPerRoute)
+ .build());
}
public AbstractRestClient(String url, String user, String password,
- int timeout, int maxTotal, int maxPerRoute) {
- this(url, OkHttpConfig.builder()
- .user(user).password(password)
- .timeout(timeout)
- .maxTotal(maxTotal)
- .maxPerRoute(maxPerRoute)
- .build());
+ int timeout, int maxConns, int maxConnsPerRoute)
{
+ this(url, RestClientConfig.builder()
+ .user(user)
+ .password(password)
+ .timeout(timeout)
+ .maxConns(maxConns)
+ .maxConnsPerRoute(maxConnsPerRoute)
+ .build());
}
public AbstractRestClient(String url, String user, String password,
- int timeout, int maxTotal, int maxPerRoute,
+ int timeout, int maxConns, int maxConnsPerRoute,
String trustStoreFile,
String trustStorePassword) {
- this(url, OkHttpConfig.builder()
- .user(user).password(password)
- .timeout(timeout)
- .maxTotal(maxTotal)
- .maxPerRoute(maxPerRoute)
- .trustStoreFile(trustStoreFile)
- .trustStorePassword(trustStorePassword)
- .build());
- }
-
- public AbstractRestClient(String url, String token, Integer timeout) {
- this(url, OkHttpConfig.builder()
- .token(token)
- .timeout(timeout)
- .build());
- }
-
- public AbstractRestClient(String url, String token, Integer timeout,
- Integer maxTotal, Integer maxPerRoute) {
- this(url, OkHttpConfig.builder()
- .token(token)
- .timeout(timeout)
- .maxTotal(maxTotal)
- .maxPerRoute(maxPerRoute)
- .build());
- }
-
- public AbstractRestClient(String url, String token, Integer timeout,
- Integer maxTotal, Integer maxPerRoute,
+ this(url, RestClientConfig.builder()
+ .user(user).password(password)
+ .timeout(timeout)
+ .maxConns(maxConns)
+ .maxConnsPerRoute(maxConnsPerRoute)
+ .trustStoreFile(trustStoreFile)
+ .trustStorePassword(trustStorePassword)
+ .build());
+ }
+
+ public AbstractRestClient(String url, String token, int timeout) {
+ this(url, RestClientConfig.builder()
+ .token(token)
+ .timeout(timeout)
+ .build());
+ }
+
+ public AbstractRestClient(String url, String token, int timeout,
+ int maxConns, int maxConnsPerRoute) {
+ this(url, RestClientConfig.builder()
+ .token(token)
+ .timeout(timeout)
+ .maxConns(maxConns)
+ .maxConnsPerRoute(maxConnsPerRoute)
+ .build());
+ }
+
+ public AbstractRestClient(String url, String token, int timeout,
Review Comment:
it's too many constructors, we can remove this method
##########
hugegraph-common/src/main/java/org/apache/hugegraph/rest/AbstractRestClient.java:
##########
@@ -60,92 +60,92 @@ public abstract class AbstractRestClient implements
RestClient {
private final Request.Builder requestBuilder;
public AbstractRestClient(String url, int timeout) {
- this(url, OkHttpConfig.builder()
- .timeout(timeout)
- .build());
+ this(url, RestClientConfig.builder().timeout(timeout).build());
}
public AbstractRestClient(String url, String user, String password,
- Integer timeout) {
- this(url, OkHttpConfig.builder()
- .user(user).password(password)
- .timeout(timeout)
- .build());
+ int timeout) {
+ this(url, RestClientConfig.builder()
+ .user(user)
+ .password(password)
+ .timeout(timeout)
+ .build());
}
public AbstractRestClient(String url, int timeout,
- int maxTotal, int maxPerRoute) {
- this(url, null, null, timeout, maxTotal, maxPerRoute);
+ int maxConns, int maxConnsPerRoute) {
+ this(url, null, null, timeout, maxConns, maxConnsPerRoute);
}
public AbstractRestClient(String url, int timeout, int idleTime,
- int maxTotal, int maxPerRoute) {
- this(url, OkHttpConfig.builder()
- .idleTime(idleTime)
- .timeout(timeout)
- .maxTotal(maxTotal)
- .maxPerRoute(maxPerRoute)
- .build());
+ int maxConns, int maxConnsPerRoute) {
+ this(url, RestClientConfig.builder()
+ .idleTime(idleTime)
+ .timeout(timeout)
+ .maxConns(maxConns)
+ .maxConnsPerRoute(maxConnsPerRoute)
+ .build());
}
public AbstractRestClient(String url, String user, String password,
- int timeout, int maxTotal, int maxPerRoute) {
- this(url, OkHttpConfig.builder()
- .user(user).password(password)
- .timeout(timeout)
- .maxTotal(maxTotal)
- .maxPerRoute(maxPerRoute)
- .build());
+ int timeout, int maxConns, int maxConnsPerRoute)
{
+ this(url, RestClientConfig.builder()
+ .user(user)
+ .password(password)
+ .timeout(timeout)
+ .maxConns(maxConns)
+ .maxConnsPerRoute(maxConnsPerRoute)
+ .build());
}
public AbstractRestClient(String url, String user, String password,
- int timeout, int maxTotal, int maxPerRoute,
+ int timeout, int maxConns, int maxConnsPerRoute,
String trustStoreFile,
String trustStorePassword) {
- this(url, OkHttpConfig.builder()
- .user(user).password(password)
- .timeout(timeout)
- .maxTotal(maxTotal)
- .maxPerRoute(maxPerRoute)
- .trustStoreFile(trustStoreFile)
- .trustStorePassword(trustStorePassword)
- .build());
- }
-
- public AbstractRestClient(String url, String token, Integer timeout) {
- this(url, OkHttpConfig.builder()
- .token(token)
- .timeout(timeout)
- .build());
- }
-
- public AbstractRestClient(String url, String token, Integer timeout,
- Integer maxTotal, Integer maxPerRoute) {
- this(url, OkHttpConfig.builder()
- .token(token)
- .timeout(timeout)
- .maxTotal(maxTotal)
- .maxPerRoute(maxPerRoute)
- .build());
- }
-
- public AbstractRestClient(String url, String token, Integer timeout,
- Integer maxTotal, Integer maxPerRoute,
+ this(url, RestClientConfig.builder()
+ .user(user).password(password)
+ .timeout(timeout)
+ .maxConns(maxConns)
+ .maxConnsPerRoute(maxConnsPerRoute)
+ .trustStoreFile(trustStoreFile)
+ .trustStorePassword(trustStorePassword)
+ .build());
+ }
+
+ public AbstractRestClient(String url, String token, int timeout) {
+ this(url, RestClientConfig.builder()
+ .token(token)
+ .timeout(timeout)
+ .build());
+ }
+
+ public AbstractRestClient(String url, String token, int timeout,
Review Comment:
it's too many constructors, we can remove this method
##########
hugegraph-common/src/main/java/org/apache/hugegraph/rest/AbstractRestClient.java:
##########
@@ -60,92 +60,92 @@ public abstract class AbstractRestClient implements
RestClient {
private final Request.Builder requestBuilder;
public AbstractRestClient(String url, int timeout) {
- this(url, OkHttpConfig.builder()
- .timeout(timeout)
- .build());
+ this(url, RestClientConfig.builder().timeout(timeout).build());
}
public AbstractRestClient(String url, String user, String password,
- Integer timeout) {
- this(url, OkHttpConfig.builder()
- .user(user).password(password)
- .timeout(timeout)
- .build());
+ int timeout) {
+ this(url, RestClientConfig.builder()
+ .user(user)
+ .password(password)
+ .timeout(timeout)
+ .build());
}
public AbstractRestClient(String url, int timeout,
- int maxTotal, int maxPerRoute) {
- this(url, null, null, timeout, maxTotal, maxPerRoute);
+ int maxConns, int maxConnsPerRoute) {
Review Comment:
it's too many constructors, we can remove this method
--
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.
To unsubscribe, e-mail: dev-unsubscr...@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
