feat(client): replace jersey with okhttp in hugegraph-client (Draft PR) #508

zhenyuT · 2023-08-14T16:51:32Z

[Feature] Replace jersey in hugegraph-client/common to avoid SpringBoot/Jarkata conflicts #505

1、直接修改hugegraph-client代码，覆盖common中的类
2、对于https自定义证书的实现暂时缺失
3、运行ci失败，且耗时较久(30min)，部分单测执行出现connection timeout的异常。推测可能okhttp client的close存在问题，多次创建client实例导致内存/连接泄漏，导致存在性能问题

麻烦先看下代码，如果大体方向没问题的话，我再继续进行代码优化 (以及调整 hugegrpah-common 中的依赖, 目前是直接在 client 调整)

imbajin · 2023-08-15T01:44:46Z

thanks and some todos：

seems also need handle https test in ci（loader/hubble..）？
fix dependencies check as the contribution doc example in website

simon824

Overall looks good, but we need fix ci first and improve codestyle refer to https://github.com/apache/incubator-hugegraph/blob/master/hugegraph-style.xml

hugegraph-client/src/main/java/org/apache/hugegraph/rest/BasicAuthInterceptor.java

hugegraph-client/src/main/java/org/apache/hugegraph/rest/OkhttpConfig.java

hugegraph-client/src/main/java/org/apache/hugegraph/rest/TokenInterceptor.java

hugegraph-client/src/test/java/org/apache/hugegraph/BaseClientTest.java

zhenyuT · 2023-08-16T17:32:51Z

Overall looks good, but we need fix ci first and improve codestyle refer to https://github.com/apache/incubator-hugegraph/blob/master/hugegraph-style.xml

I've done my best to fix the issues mentioned on,but there is one more problem. When I add the licence header in the file "hugegraph-client/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker",the unit test will run error.

codecov · 2023-08-16T17:39:10Z

Codecov Report

Merging #508 (d3d3daf) into master (d9f234a) will increase coverage by 0.18%.
The diff coverage is 77.36%.

@@             Coverage Diff              @@
##             master     #508      +/-   ##
============================================
+ Coverage     62.42%   62.60%   +0.18%     
- Complexity      895     1937    +1042     
============================================
  Files            91      265     +174     
  Lines          4404     9689    +5285     
  Branches        519      900     +381     
============================================
+ Hits           2749     6066    +3317     
- Misses         1450     3228    +1778     
- Partials        205      395     +190

Files Changed	Coverage Δ
...va/org/apache/hugegraph/api/gremlin/CypherAPI.java	`50.00% <0.00%> (ø)`
...n/java/org/apache/hugegraph/driver/HugeClient.java	`82.08% <0.00%> (ø)`
.../apache/hugegraph/rest/OkhttpTokenInterceptor.java	`0.00% <0.00%> (ø)`
...ava/org/apache/hugegraph/api/graphs/GraphsAPI.java	`67.39% <66.66%> (ø)`
...va/org/apache/hugegraph/rest/OkhttpRestResult.java	`67.64% <67.64%> (ø)`
...pache/hugegraph/rest/OkhttpAbstractRestClient.java	`68.68% <68.68%> (ø)`
...java/org/apache/hugegraph/api/graph/VertexAPI.java	`77.35% <81.81%> (ø)`
...n/java/org/apache/hugegraph/rest/OkhttpConfig.java	`90.90% <90.90%> (ø)`
.../java/org/apache/hugegraph/api/auth/AccessAPI.java	`100.00% <100.00%> (ø)`
.../java/org/apache/hugegraph/api/auth/BelongAPI.java	`100.00% <100.00%> (ø)`
... and 45 more

... and 119 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

imbajin · 2023-08-16T17:39:42Z

Overall looks good, but we need fix ci first and improve codestyle refer to apache/incubator-hugegraph@master/hugegraph-style.xml

I've done my best to fix the issues mentioned on,but there is one more problem. When I add the licence header in the file "hugegraph-client/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker",the unit test will run error.

if u point this, exclude it in plugin settings if we really don't need or can't add license header in it (also check maven-rat in pom.xml)

imbajin · 2023-08-16T18:04:48Z

hugegraph-client/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker

@@ -0,0 +1 @@
+mock-maker-inline


maybe we could try to add license header for it first if (it's) necessary

Suggested change

mock-maker-inline

# license header with `#` comment

# ....

# end license

mock-maker-inline

maybe we could try to add license header for it first if (it's) necessary

This just works fine, thank you.

javeme

Thanks for your contribution

javeme · 2023-08-17T12:48:37Z

hugegraph-client/src/main/java/org/apache/hugegraph/api/graph/GraphAPI.java

-        return UriComponent.encode(raw, Type.QUERY_PARAM_SPACE_ENCODED);
-    }
+//    public static String encode(String raw) {
+//        return UriComponent.encode(raw, Type.QUERY_PARAM_SPACE_ENCODED);


any alternative plan?

any alternative plan?

看原来代码的意图，是要对url中的参数进行编码。但是okhttp中使用HttpUrl.Builder构建url，其实会自动进行编码的，所以感觉没必要由使用方对参数编码后再传参

javeme · 2023-08-17T12:51:51Z

hugegraph-client/src/test/java/org/apache/hugegraph/unit/RestResultTest.java

@@ -55,7 +56,7 @@

 public class RestResultTest extends BaseUnitTest {

-    private jakarta.ws.rs.core.Response mockResponse;
+    private okhttp3.Response mockResponse;


just import the Response class?

这里之前用的jakartar包中的Response，现在实现改为okhttp，因此对应mock的对象也修改为okhttp包中的Response。这样做是存在什么问题？

ok, I mean we can try to import okhttp3.Response

javeme · 2023-08-17T13:07:34Z

麻烦先看下代码，如果大体方向没问题的话，我再继续进行代码优化 (以及调整 hugegrpah-common 中的依赖, 目前是直接在 client 调整)

整体方案没问题的。
CI失败，看了下是超时问题SocketTimeoutException，请确保各类超时时间设置正确了：

MultiNodeShortestPathApiTest Time elapsed: 10.038 s  <<< ERROR!
java.net.SocketTimeoutException: timeout

zhenyuT · 2023-08-17T15:01:25Z

麻烦先看下代码，如果大体方向没问题的话，我再继续进行代码优化 (以及调整 hugegrpah-common 中的依赖, 目前是直接在 client 调整)

整体方案没问题的。 CI失败，看了下是超时问题SocketTimeoutException，请确保各类超时时间设置正确了：
MultiNodeShortestPathApiTest Time elapsed: 10.038 s  <<< ERROR!
java.net.SocketTimeoutException: timeout

有点奇怪，我fork到自己仓库，执行是成功的，没有遇到超时的问题。两边执行是有什么区别吗？

z7658329 · 2023-08-23T02:25:26Z

seems good, do you have test introducing the new hugegrap-client dependency in a spring boot project and ensured that it starts up correctly? and you need to cover two scenarios: jdk8/jdk11 + springboot2.X and jdk17 + springboot3, you can refer 495 @zhenyuT

zhenyuT · 2023-08-27T17:29:25Z

seems good, do you have test introducing the new hugegrap-client dependency in a spring boot project and ensured that it starts up correctly? and you need to cover two scenarios: jdk8/jdk11 + springboot2.X and jdk17 + springboot3, you can refer 495 @zhenyuT

I've test with springboot2.X and springboot3,but just simple test.

z7658329 · 2023-08-30T08:59:55Z

hugegraph-client/pom.xml

@@ -37,28 +37,73 @@
    </description>

    <properties>
+        <lombok.version>1.18.8</lombok.version>
+        <okhttp.version>4.10.0</okhttp.version>


shoule extract dependency version to root pom.xml， sub module just import?

shoule extract dependency version to root pom.xml， sub module just import?

seems no need, only extract dependencies that multiple modules need to use (leave single usage alone?)

simon824 · 2023-09-05T04:15:37Z

LGTM, @zhenyuT please resolve conflicts when you are free.

javeme · 2023-09-08T16:30:54Z

hugegraph-client/src/main/java/org/apache/hugegraph/api/auth/UserAPI.java

        return result.readObject(User.class);
    }

    public UserRole getUserRole(Object id) {
-        String idEncoded = RestClient.encode(formatEntityId(id));
+//        String idEncoded = RestClient.encode(formatEntityId(id));


can we remove the unused code

javeme · 2023-09-08T16:31:01Z

hugegraph-client/src/main/java/org/apache/hugegraph/api/graph/EdgeAPI.java

-        MultivaluedHashMap<String, Object> headers = new MultivaluedHashMap<>();
-        headers.putSingle("Content-Encoding", BATCH_ENCODING);
+//        MultivaluedHashMap<String, Object> headers = new MultivaluedHashMap<>();
+//        headers.putSingle("Content-Encoding", BATCH_ENCODING);


javeme · 2023-09-08T16:31:11Z

hugegraph-client/src/main/java/org/apache/hugegraph/api/graph/EdgeAPI.java

-        headers.putSingle("Content-Encoding", BATCH_ENCODING);
-        RestResult result = this.client.put(this.batchPath(), null,
+//        MultivaluedHashMap<String, Object> headers = new MultivaluedHashMap<>();
+//        headers.putSingle("Content-Encoding", BATCH_ENCODING);


javeme · 2023-09-08T16:31:35Z

hugegraph-client/src/main/java/org/apache/hugegraph/api/graph/EdgeAPI.java

-        RestResult result = this.client.put(this.batchPath(), null,
+//        MultivaluedHashMap<String, Object> headers = new MultivaluedHashMap<>();
+//        headers.putSingle("Content-Encoding", BATCH_ENCODING);
+        Headers headers = new Headers.Builder().add("Content-Encoding", BATCH_ENCODING).build();


define a const?

javeme · 2023-09-08T16:34:16Z

hugegraph-client/src/main/java/org/apache/hugegraph/api/graph/VertexAPI.java

-        MultivaluedHashMap<String, Object> headers = new MultivaluedHashMap<>();
-        headers.putSingle("Content-Encoding", BATCH_ENCODING);
+//        MultivaluedHashMap<String, Object> headers = new MultivaluedHashMap<>();
+//        headers.putSingle("Content-Encoding", BATCH_ENCODING);


javeme · 2023-09-08T16:37:50Z

hugegraph-client/src/main/java/org/apache/hugegraph/rest/OkhttpConfig.java

+@Builder
+@Getter
+@Setter
+public class OkhttpConfig {


expect a blank line after the class define

javeme · 2023-09-08T16:38:14Z

hugegraph-client/src/main/java/org/apache/hugegraph/rest/OkhttpRestClient.java

+
+import java.util.Map;
+
+public interface OkhttpRestClient {


don't need to Okhttp prefix, we expect to keep the same interface name

javeme · 2023-09-08T16:39:08Z

hugegraph-client/src/main/java/org/apache/hugegraph/rest/OkhttpRestResult.java

+import java.util.ArrayList;
+import java.util.List;
+
+public class OkhttpRestResult {


don't need to Okhttp prefix

javeme · 2023-09-08T16:41:00Z

hugegraph-client/src/test/java/org/apache/hugegraph/unit/RestResultTest.java

@@ -55,7 +56,7 @@

 public class RestResultTest extends BaseUnitTest {

-    private jakarta.ws.rs.core.Response mockResponse;
+    private okhttp3.Response mockResponse;


ok, I mean we can try to import okhttp3.Response

javeme · 2023-09-08T16:41:39Z

pom.xml

+<!--                <version>${jersey.version}</version>-->
+<!--                <type>pom</type>-->
+<!--                <scope>import</scope>-->
+<!--            </dependency>-->


we can remove all the unused code

javeme · 2023-09-10T03:00:58Z

implemented by apache/incubator-hugegraph-commons#133

github-actions · 2023-10-10T21:00:32Z

Due to the lack of activity, the current pr is marked as stale and will be closed after 180 days, any update will remove the stale label

github-actions bot added the client hugegraph-client label Aug 14, 2023

simon824 reviewed Aug 15, 2023

View reviewed changes

imbajin requested review from z7658329 and javeme August 16, 2023 17:32

imbajin reviewed Aug 16, 2023

View reviewed changes

javeme reviewed Aug 17, 2023

View reviewed changes

github-actions bot added the hubble hugegraph-hubble label Aug 17, 2023

zhenyuT mentioned this pull request Aug 27, 2023

feat(common): replace jersey dependencies with OkHttp (Breaking Change) apache/incubator-hugegraph-commons#133

Merged

z7658329 reviewed Aug 30, 2023

View reviewed changes

javeme reviewed Sep 8, 2023

View reviewed changes

github-actions bot added the inactive label Oct 10, 2023

zhenyuT closed this Nov 22, 2023

zhenyuT force-pushed the master branch from d3d3daf to 6d0b01b Compare November 22, 2023 12:41

zhenyuT mentioned this pull request Nov 23, 2023

feat: update common version and remove jersey code #538

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(client): replace jersey with okhttp in hugegraph-client (Draft PR) #508

feat(client): replace jersey with okhttp in hugegraph-client (Draft PR) #508

zhenyuT commented Aug 14, 2023 •

edited by imbajin

Loading

imbajin commented Aug 15, 2023 •

edited

Loading

simon824 left a comment

zhenyuT commented Aug 16, 2023

codecov bot commented Aug 16, 2023 •

edited

Loading

imbajin commented Aug 16, 2023

imbajin Aug 16, 2023 •

edited

Loading

zhenyuT Aug 17, 2023

javeme left a comment

javeme Aug 17, 2023

zhenyuT Aug 27, 2023

javeme Aug 17, 2023

zhenyuT Aug 27, 2023

javeme Sep 8, 2023

javeme commented Aug 17, 2023

zhenyuT commented Aug 17, 2023

z7658329 commented Aug 23, 2023

zhenyuT commented Aug 27, 2023

z7658329 Aug 30, 2023

imbajin Aug 30, 2023

simon824 commented Sep 5, 2023

javeme Sep 8, 2023

javeme Sep 8, 2023

javeme Sep 8, 2023

javeme Sep 8, 2023

javeme Sep 8, 2023

javeme Sep 8, 2023

javeme Sep 8, 2023

javeme Sep 8, 2023

javeme Sep 8, 2023

javeme Sep 8, 2023

javeme commented Sep 10, 2023

github-actions bot commented Oct 10, 2023

feat(client): replace jersey with okhttp in hugegraph-client (Draft PR) #508

feat(client): replace jersey with okhttp in hugegraph-client (Draft PR) #508

Conversation

zhenyuT commented Aug 14, 2023 • edited by imbajin Loading

imbajin commented Aug 15, 2023 • edited Loading

simon824 left a comment

Choose a reason for hiding this comment

zhenyuT commented Aug 16, 2023

codecov bot commented Aug 16, 2023 • edited Loading

Codecov Report

imbajin commented Aug 16, 2023

imbajin Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javeme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javeme commented Aug 17, 2023

zhenyuT commented Aug 17, 2023

z7658329 commented Aug 23, 2023

zhenyuT commented Aug 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simon824 commented Sep 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javeme commented Sep 10, 2023

github-actions bot commented Oct 10, 2023

zhenyuT commented Aug 14, 2023 •

edited by imbajin

Loading

imbajin commented Aug 15, 2023 •

edited

Loading

codecov bot commented Aug 16, 2023 •

edited

Loading

imbajin Aug 16, 2023 •

edited

Loading