javeme commented on code in PR #133:
URL:
https://github.com/apache/incubator-hugegraph-commons/pull/133#discussion_r1398113866
##########
hugegraph-common/src/main/java/org/apache/hugegraph/rest/RestClientConfig.java:
##########
@@ -33,7 +35,8 @@ public class RestClientConfig {
private Integer maxConns;
private Integer maxConnsPerRoute;
private Integer idleTime = 5;
- private Integer maxIdleConnections = 5;
+ private TimeUnit idleTimeUnit = TimeUnit.MINUTES;
Review Comment:
sorry I means add some comment like:
```
// unit in seconds
private Integer idleTime = 5;
```
##########
hugegraph-common/src/main/java/org/apache/hugegraph/rest/AbstractRestClient.java:
##########
@@ -207,31 +237,66 @@ public RestResult post(String path, Object object) {
}
@Override
- public RestResult post(String path, Object object,
- MultivaluedMap<String, Object> headers) {
+ public RestResult post(String path, Object object, RestHeaders headers) {
return this.post(path, object, headers, null);
}
@Override
- public RestResult post(String path, Object object,
- Map<String, Object> params) {
+ public RestResult post(String path, Object object, Map<String, Object>
params) {
return this.post(path, object, null, params);
}
+ private Request.Builder getRequestBuilder(String path, String id,
RestHeaders headers,
Review Comment:
prefer genRequestBuilder name since it's not just a simple get-a-field method
##########
hugegraph-common/src/main/java/org/apache/hugegraph/rest/RestClientConfig.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hugegraph.rest;
+
+import java.util.concurrent.TimeUnit;
+
+import lombok.Builder;
+import lombok.Getter;
+import lombok.Setter;
+
+@Builder
+@Getter
+@Setter
+public class RestClientConfig {
+
+ private String user;
+ private String password;
+ private String token;
+ private Integer timeout;
+ private Integer maxConns;
+ private Integer maxConnsPerRoute;
+ private Integer idleTime = 5;
Review Comment:
set default value to 30s?
##########
hugegraph-common/src/main/java/org/apache/hugegraph/rest/HttpHeadersConstant.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.hugegraph.rest;
+
+public class HttpHeadersConstant {
Review Comment:
prefer to merge this class into `RestHeaders` class
##########
hugegraph-common/src/main/java/org/apache/hugegraph/rest/AbstractRestClient.java:
##########
@@ -214,10 +181,10 @@ private OkHttpClient buildOkHttpClient(RestClientConfig
config) {
.readTimeout(config.getTimeout(), TimeUnit.MILLISECONDS);
}
- if (config.getMaxIdleConnections() != null || config.getIdleTime() !=
null) {
- ConnectionPool connectionPool = new
ConnectionPool(config.getMaxIdleConnections(),
+ if (config.getMaxIdleConns() != null || config.getIdleTime() != null) {
+ ConnectionPool connectionPool = new
ConnectionPool(config.getMaxIdleConns(),
config.getIdleTime(),
-
TimeUnit.MILLISECONDS);
+
TimeUnit.MINUTES);
Review Comment:
prefer in seconds
##########
hugegraph-common/src/main/java/org/apache/hugegraph/rest/HttpHeadersConstant.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+
+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_PREFIX = "Bearer ";
+
+
Review Comment:
unexpected blank line
##########
hugegraph-common/src/main/java/org/apache/hugegraph/rest/AbstractRestClient.java:
##########
@@ -207,31 +237,66 @@ public RestResult post(String path, Object object) {
}
@Override
- public RestResult post(String path, Object object,
- MultivaluedMap<String, Object> headers) {
+ public RestResult post(String path, Object object, RestHeaders headers) {
return this.post(path, object, headers, null);
}
@Override
- public RestResult post(String path, Object object,
- Map<String, Object> params) {
+ public RestResult post(String path, Object object, Map<String, Object>
params) {
return this.post(path, object, null, params);
}
+ private Request.Builder getRequestBuilder(String path, String id,
RestHeaders headers,
+ Map<String, Object> params) {
+ HttpUrl.Builder urlBuilder =
Objects.requireNonNull(HttpUrl.parse(this.baseUrl))
+ .newBuilder()
+ .addPathSegments(path);
+ if (id != null) {
+ urlBuilder.addPathSegment(id);
+ }
+
+ if (params != null) {
+ params.forEach((name, value) -> {
+ if (value == null) {
+ return;
+ }
+
+ if (value instanceof Collection) {
+ for (Object i : (Collection<?>) value) {
+ urlBuilder.addQueryParameter(name, String.valueOf(i));
+ }
+ } else {
+ urlBuilder.addQueryParameter(name, String.valueOf(value));
+ }
+ });
+ }
+
+ Request.Builder builder = newRequestBuilder().url(urlBuilder.build());
+
+ if (headers != null) {
+ builder.headers(headers.toOkHttpHeader());
+ }
+
+ this.attachAuthToRequest(builder);
+
+ return builder;
+ }
+
+ protected Request.Builder newRequestBuilder() {
Review Comment:
can we add some comments why we add this method?
`// In order to provide subclasses with overloading opportunities`
##########
hugegraph-common/src/main/java/org/apache/hugegraph/rest/AbstractRestClient.java:
##########
@@ -17,188 +17,218 @@
package org.apache.hugegraph.rest;
+import java.io.FileInputStream;
+import java.io.IOException;
import java.net.URI;
-import java.security.KeyManagementException;
-import java.security.SecureRandom;
-import java.security.cert.CertificateException;
-import java.security.cert.X509Certificate;
+import java.security.KeyStore;
+import java.util.Arrays;
import java.util.Collection;
-import java.util.List;
import java.util.Map;
-import java.util.concurrent.Callable;
-import java.util.concurrent.ScheduledExecutorService;
+import java.util.Objects;
import java.util.concurrent.TimeUnit;
import javax.net.ssl.HostnameVerifier;
import javax.net.ssl.HttpsURLConnection;
import javax.net.ssl.SSLContext;
import javax.net.ssl.SSLSession;
+import javax.net.ssl.SSLSocketFactory;
import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
import javax.net.ssl.X509TrustManager;
-import org.apache.commons.collections.MapUtils;
-import org.apache.commons.lang.StringUtils;
-import org.apache.commons.lang3.tuple.Pair;
-import org.apache.http.HttpHeaders;
-import org.apache.http.config.Registry;
-import org.apache.http.config.RegistryBuilder;
-import org.apache.http.conn.socket.ConnectionSocketFactory;
-import org.apache.http.conn.socket.PlainConnectionSocketFactory;
-import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
-import org.apache.http.impl.conn.PoolingHttpClientConnectionManager;
-import org.apache.http.pool.PoolStats;
-import org.apache.hugegraph.util.E;
-import org.apache.hugegraph.util.ExecutorUtil;
-import org.glassfish.jersey.SslConfigurator;
-import org.glassfish.jersey.apache.connector.ApacheClientProperties;
-import org.glassfish.jersey.apache.connector.ApacheConnectorProvider;
-import org.glassfish.jersey.client.ClientConfig;
-import org.glassfish.jersey.client.ClientProperties;
-import org.glassfish.jersey.client.JerseyClientBuilder;
-import org.glassfish.jersey.client.authentication.HttpAuthenticationFeature;
-import org.glassfish.jersey.internal.util.collection.Ref;
-import org.glassfish.jersey.internal.util.collection.Refs;
-import org.glassfish.jersey.message.GZipEncoder;
-import org.glassfish.jersey.uri.UriComponent;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.hugegraph.util.JsonUtil;
+import org.jetbrains.annotations.NotNull;
import com.google.common.collect.ImmutableMap;
-import jakarta.ws.rs.client.Client;
-import jakarta.ws.rs.client.ClientRequestContext;
-import jakarta.ws.rs.client.ClientRequestFilter;
-import jakarta.ws.rs.client.Entity;
-import jakarta.ws.rs.client.Invocation.Builder;
-import jakarta.ws.rs.client.WebTarget;
-import jakarta.ws.rs.core.MediaType;
-import jakarta.ws.rs.core.MultivaluedMap;
-import jakarta.ws.rs.core.Response;
-import jakarta.ws.rs.core.Variant;
-
+import lombok.SneakyThrows;
+import okhttp3.ConnectionPool;
+import okhttp3.HttpUrl;
+import okhttp3.MediaType;
+import okhttp3.OkHttpClient;
+import okhttp3.Request;
+import okhttp3.RequestBody;
+import okhttp3.Response;
+import okio.BufferedSink;
+import okio.GzipSink;
+import okio.Okio;
+
+/**
+ * This class provides an abstract implementation of the RestClient interface.
+ * It provides methods for making HTTP requests (GET, POST, PUT, DELETE) to a
REST API.
+ * Note: It uses the OkHttp library to make these requests for now.
+ */
public abstract class AbstractRestClient implements RestClient {
- // Time unit: hours
- private static final long TTL = 24L;
- // Time unit: ms
- private static final long IDLE_TIME = 40L * 1000L;
-
- private static final String TOKEN_KEY = "tokenKey";
+ private final ThreadLocal<String> authContext;
- private final Client client;
- private final WebTarget target;
+ private final OkHttpClient client;
- private PoolingHttpClientConnectionManager pool;
- private ScheduledExecutorService cleanExecutor;
+ private final String baseUrl;
public AbstractRestClient(String url, int timeout) {
- this(url, new ConfigBuilder().configTimeout(timeout).build());
+ this(url, RestClientConfig.builder().timeout(timeout).build());
}
- public AbstractRestClient(String url, String user, String password,
- int timeout) {
- this(url, new ConfigBuilder().configTimeout(timeout)
- .configUser(user, password)
- .build());
+ public AbstractRestClient(String url, String user, String password, 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, new ConfigBuilder().configTimeout(timeout)
- .configPool(maxTotal, maxPerRoute)
- .build());
+ public AbstractRestClient(String url, int timeout, int idleTime, int
maxConns,
+ int maxConnsPerRoute) {
Review Comment:
can we keep similar params in the same line:
```java
int maxConns, int maxConnsPerRoute,
```
##########
hugegraph-common/src/main/java/org/apache/hugegraph/rest/AbstractRestClient.java:
##########
@@ -17,188 +17,218 @@
package org.apache.hugegraph.rest;
+import java.io.FileInputStream;
+import java.io.IOException;
import java.net.URI;
-import java.security.KeyManagementException;
-import java.security.SecureRandom;
-import java.security.cert.CertificateException;
-import java.security.cert.X509Certificate;
+import java.security.KeyStore;
+import java.util.Arrays;
import java.util.Collection;
-import java.util.List;
import java.util.Map;
-import java.util.concurrent.Callable;
-import java.util.concurrent.ScheduledExecutorService;
+import java.util.Objects;
import java.util.concurrent.TimeUnit;
import javax.net.ssl.HostnameVerifier;
import javax.net.ssl.HttpsURLConnection;
import javax.net.ssl.SSLContext;
import javax.net.ssl.SSLSession;
+import javax.net.ssl.SSLSocketFactory;
import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
import javax.net.ssl.X509TrustManager;
-import org.apache.commons.collections.MapUtils;
-import org.apache.commons.lang.StringUtils;
-import org.apache.commons.lang3.tuple.Pair;
-import org.apache.http.HttpHeaders;
-import org.apache.http.config.Registry;
-import org.apache.http.config.RegistryBuilder;
-import org.apache.http.conn.socket.ConnectionSocketFactory;
-import org.apache.http.conn.socket.PlainConnectionSocketFactory;
-import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
-import org.apache.http.impl.conn.PoolingHttpClientConnectionManager;
-import org.apache.http.pool.PoolStats;
-import org.apache.hugegraph.util.E;
-import org.apache.hugegraph.util.ExecutorUtil;
-import org.glassfish.jersey.SslConfigurator;
-import org.glassfish.jersey.apache.connector.ApacheClientProperties;
-import org.glassfish.jersey.apache.connector.ApacheConnectorProvider;
-import org.glassfish.jersey.client.ClientConfig;
-import org.glassfish.jersey.client.ClientProperties;
-import org.glassfish.jersey.client.JerseyClientBuilder;
-import org.glassfish.jersey.client.authentication.HttpAuthenticationFeature;
-import org.glassfish.jersey.internal.util.collection.Ref;
-import org.glassfish.jersey.internal.util.collection.Refs;
-import org.glassfish.jersey.message.GZipEncoder;
-import org.glassfish.jersey.uri.UriComponent;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.hugegraph.util.JsonUtil;
+import org.jetbrains.annotations.NotNull;
import com.google.common.collect.ImmutableMap;
-import jakarta.ws.rs.client.Client;
-import jakarta.ws.rs.client.ClientRequestContext;
-import jakarta.ws.rs.client.ClientRequestFilter;
-import jakarta.ws.rs.client.Entity;
-import jakarta.ws.rs.client.Invocation.Builder;
-import jakarta.ws.rs.client.WebTarget;
-import jakarta.ws.rs.core.MediaType;
-import jakarta.ws.rs.core.MultivaluedMap;
-import jakarta.ws.rs.core.Response;
-import jakarta.ws.rs.core.Variant;
-
+import lombok.SneakyThrows;
+import okhttp3.ConnectionPool;
+import okhttp3.HttpUrl;
+import okhttp3.MediaType;
+import okhttp3.OkHttpClient;
+import okhttp3.Request;
+import okhttp3.RequestBody;
+import okhttp3.Response;
+import okio.BufferedSink;
+import okio.GzipSink;
+import okio.Okio;
+
+/**
+ * This class provides an abstract implementation of the RestClient interface.
+ * It provides methods for making HTTP requests (GET, POST, PUT, DELETE) to a
REST API.
+ * Note: It uses the OkHttp library to make these requests for now.
+ */
public abstract class AbstractRestClient implements RestClient {
- // Time unit: hours
- private static final long TTL = 24L;
- // Time unit: ms
- private static final long IDLE_TIME = 40L * 1000L;
-
- private static final String TOKEN_KEY = "tokenKey";
+ private final ThreadLocal<String> authContext;
- private final Client client;
- private final WebTarget target;
+ private final OkHttpClient client;
- private PoolingHttpClientConnectionManager pool;
- private ScheduledExecutorService cleanExecutor;
+ private final String baseUrl;
public AbstractRestClient(String url, int timeout) {
- this(url, new ConfigBuilder().configTimeout(timeout).build());
+ this(url, RestClientConfig.builder().timeout(timeout).build());
}
- public AbstractRestClient(String url, String user, String password,
- int timeout) {
- this(url, new ConfigBuilder().configTimeout(timeout)
- .configUser(user, password)
- .build());
+ public AbstractRestClient(String url, String user, String password, 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, new ConfigBuilder().configTimeout(timeout)
- .configPool(maxTotal, maxPerRoute)
- .build());
+ public AbstractRestClient(String url, int timeout, int idleTime, int
maxConns,
+ int maxConnsPerRoute) {
+ this(url, RestClientConfig.builder()
+ .idleTime(idleTime)
+ .timeout(timeout)
+ .maxConns(maxConns)
+ .maxConnsPerRoute(maxConnsPerRoute)
+ .build());
}
- public AbstractRestClient(String url, int timeout, int idleTime,
- int maxTotal, int maxPerRoute) {
- this(url, new ConfigBuilder().configTimeout(timeout)
- .configIdleTime(idleTime)
- .configPool(maxTotal, maxPerRoute)
- .build());
- }
+ public AbstractRestClient(String url, String user, String password, int
timeout, int maxConns,
Review Comment:
can we keep similar params in the same line:
```java
int maxConns, int maxConnsPerRoute,
String trustStoreFile, String trustStorePassword
```
--
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
