You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hugegraph.apache.org by "zhenyuT (via GitHub)" <gi...@apache.org> on 2023/08/14 16:51:34 UTC
[GitHub] [incubator-hugegraph-toolchain] zhenyuT opened a new pull request, #508: feat(client): replace jersey with okhttp in hugegraph-client (Draft PR)
zhenyuT opened a new pull request, #508:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/508
- [https://github.com/apache/incubator-hugegraph-toolchain/issues/505](https://github.com/apache/incubator-hugegraph-toolchain/issues/505)
1、直接修改hugegraph-client代码，覆盖common中的类
2、对于https自定义证书的实现暂时缺失
3、运行ci失败，且耗时较久(30min)，部分单测执行出现connection timeout的异常。推测可能okhttp client的close存在问题，多次创建client实例导致内存/连接泄漏，导致存在性能问题
麻烦先看下代码，如果大体方向没问题的话，我再继续进行代码优化
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-toolchain] javeme commented on a diff in pull request #508: feat(client): replace jersey with okhttp in hugegraph-client (Draft PR)
Posted by "javeme (via GitHub)" <gi...@apache.org>.
javeme commented on code in PR #508:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/508#discussion_r1297174918
##########
hugegraph-client/src/main/java/org/apache/hugegraph/api/graph/GraphAPI.java:
##########
@@ -76,10 +73,11 @@ public static String formatProperties(Map<String, Object> properties) {
* which will invalidate the jersey's automatic decoding
* because it considers the space to be encoded as `%2F`
*/
- return encode(json);
+// return encode(json);
+ return json;
}
- public static String encode(String raw) {
- return UriComponent.encode(raw, Type.QUERY_PARAM_SPACE_ENCODED);
- }
+// public static String encode(String raw) {
+// return UriComponent.encode(raw, Type.QUERY_PARAM_SPACE_ENCODED);
Review Comment:
any alternative plan?
##########
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;
Review Comment:
just import the Response class?
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] feat(client): replace jersey with okhttp in hugegraph-client (Draft PR) [incubator-hugegraph-toolchain]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #508:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/508#issuecomment-1756249461
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
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-toolchain] imbajin commented on a diff in pull request #508: feat(client): replace jersey with okhttp in hugegraph-client (Draft PR)
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #508:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/508#discussion_r1296264741
##########
hugegraph-client/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker:
##########
@@ -0,0 +1 @@
+mock-maker-inline
Review Comment:
maybe we could try to **add license header** for it first if (this file) necessary
```suggestion
# license header with `#` comment
# ....
# end license
mock-maker-inline
```
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-toolchain] imbajin commented on a diff in pull request #508: feat(client): replace jersey with okhttp in hugegraph-client (Draft PR)
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #508:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/508#discussion_r1296264741
##########
hugegraph-client/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker:
##########
@@ -0,0 +1 @@
+mock-maker-inline
Review Comment:
maybe we could try to **add license header** for it first if (it's) necessary
```suggestion
# license header with `#` comment
# ....
# end license
mock-maker-inline
```
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-toolchain] zhenyuT commented on a diff in pull request #508: feat(client): replace jersey with okhttp in hugegraph-client (Draft PR)
Posted by "zhenyuT (via GitHub)" <gi...@apache.org>.
zhenyuT commented on code in PR #508:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/508#discussion_r1296589190
##########
hugegraph-client/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker:
##########
@@ -0,0 +1 @@
+mock-maker-inline
Review Comment:
> maybe we could try to **add license header** for it first if (it's) necessary
This just works fine, thank you.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-toolchain] zhenyuT commented on a diff in pull request #508: feat(client): replace jersey with okhttp in hugegraph-client (Draft PR)
Posted by "zhenyuT (via GitHub)" <gi...@apache.org>.
zhenyuT commented on code in PR #508:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/508#discussion_r1306700778
##########
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;
Review Comment:
>
这里之前用的jakartar包中的Response，现在实现改为okhttp，因此对应mock的对象也修改为okhttp包中的Response。这样做是存在什么问题？
##########
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;
Review Comment:
>
这里之前用的jakartar包中的Response，现在实现改为okhttp，因此对应mock的对象也修改为okhttp包中的Response。这样做是存在什么问题？
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] feat(client): replace jersey with okhttp in hugegraph-client (Draft PR) [incubator-hugegraph-toolchain]
Posted by "zhenyuT (via GitHub)" <gi...@apache.org>.
zhenyuT closed pull request #508: feat(client): replace jersey with okhttp in hugegraph-client (Draft PR)
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/508
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-toolchain] imbajin commented on pull request #508: feat(client): replace jersey with okhttp in hugegraph-client (Draft PR)
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on PR #508:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/508#issuecomment-1678310782
thanks and some todos：
- [ ] seems also need handle https test in ci（loader/hubble..）？
- [ ] fix dependencies check as the contribution doc example in website
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-toolchain] zhenyuT commented on pull request #508: feat(client): replace jersey with okhttp in hugegraph-client (Draft PR)
Posted by "zhenyuT (via GitHub)" <gi...@apache.org>.
zhenyuT commented on PR #508:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/508#issuecomment-1681016340
> 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.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-toolchain] codecov[bot] commented on pull request #508: feat(client): replace jersey with okhttp in hugegraph-client (Draft PR)
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #508:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/508#issuecomment-1681025906
## [Codecov](https://app.codecov.io/gh/apache/incubator-hugegraph-toolchain/pull/508?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#508](https://app.codecov.io/gh/apache/incubator-hugegraph-toolchain/pull/508?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (9d211f8) into [master](https://app.codecov.io/gh/apache/incubator-hugegraph-toolchain/commit/d9f234ae97107f22cb020dcef073d1b8596b6ed7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (d9f234a) will **not change** coverage.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #508 +/- ##
=========================================
Coverage 62.42% 62.42%
Complexity 895 895
=========================================
Files 91 91
Lines 4404 4404
Branches 519 519
=========================================
Hits 2749 2749
Misses 1450 1450
Partials 205 205
```
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-toolchain] zhenyuT commented on a diff in pull request #508: feat(client): replace jersey with okhttp in hugegraph-client (Draft PR)
Posted by "zhenyuT (via GitHub)" <gi...@apache.org>.
zhenyuT commented on code in PR #508:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/508#discussion_r1306701074
##########
hugegraph-client/src/main/java/org/apache/hugegraph/api/graph/GraphAPI.java:
##########
@@ -76,10 +73,11 @@ public static String formatProperties(Map<String, Object> properties) {
* which will invalidate the jersey's automatic decoding
* because it considers the space to be encoded as `%2F`
*/
- return encode(json);
+// return encode(json);
+ return json;
}
- public static String encode(String raw) {
- return UriComponent.encode(raw, Type.QUERY_PARAM_SPACE_ENCODED);
- }
+// public static String encode(String raw) {
+// return UriComponent.encode(raw, Type.QUERY_PARAM_SPACE_ENCODED);
Review Comment:
> any alternative plan?
看原来代码的意图，是要对url中的参数进行编码。但是okhttp中使用HttpUrl.Builder构建url，其实会自动进行编码的，所以感觉没必要由使用方对参数编码后再传参
##########
hugegraph-client/src/main/java/org/apache/hugegraph/api/graph/GraphAPI.java:
##########
@@ -76,10 +73,11 @@ public static String formatProperties(Map<String, Object> properties) {
* which will invalidate the jersey's automatic decoding
* because it considers the space to be encoded as `%2F`
*/
- return encode(json);
+// return encode(json);
+ return json;
}
- public static String encode(String raw) {
- return UriComponent.encode(raw, Type.QUERY_PARAM_SPACE_ENCODED);
- }
+// public static String encode(String raw) {
+// return UriComponent.encode(raw, Type.QUERY_PARAM_SPACE_ENCODED);
Review Comment:
> any alternative plan?
看原来代码的意图，是要对url中的参数进行编码。但是okhttp中使用HttpUrl.Builder构建url，其实会自动进行编码的，所以感觉没必要由使用方对参数编码后再传参
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-toolchain] zhenyuT commented on pull request #508: feat(client): replace jersey with okhttp in hugegraph-client (Draft PR)
Posted by "zhenyuT (via GitHub)" <gi...@apache.org>.
zhenyuT commented on PR #508:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/508#issuecomment-1694720905
> 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](https://github.com/apache/incubator-hugegraph-toolchain/issues/495) @zhenyuT
I've test with springboot2.X and springboot3,but just simple 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.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-toolchain] javeme commented on pull request #508: feat(client): replace jersey with okhttp in hugegraph-client (Draft PR)
Posted by "javeme (via GitHub)" <gi...@apache.org>.
javeme commented on PR #508:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/508#issuecomment-1712696091
implemented by https://github.com/apache/incubator-hugegraph-commons/pull/133
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-toolchain] z7658329 commented on a diff in pull request #508: feat(client): replace jersey with okhttp in hugegraph-client (Draft PR)
Posted by "z7658329 (via GitHub)" <gi...@apache.org>.
z7658329 commented on code in PR #508:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/508#discussion_r1309937223
##########
hugegraph-client/pom.xml:
##########
@@ -37,28 +37,73 @@
</description>
<properties>
+ <lombok.version>1.18.8</lombok.version>
+ <okhttp.version>4.10.0</okhttp.version>
Review Comment:
shoule extract dependency version to root pom.xml， sub module just import?
##########
hugegraph-client/pom.xml:
##########
@@ -37,28 +37,73 @@
</description>
<properties>
+ <lombok.version>1.18.8</lombok.version>
+ <okhttp.version>4.10.0</okhttp.version>
Review Comment:
shoule extract dependency version to root pom.xml， sub module just 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.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-toolchain] imbajin commented on pull request #508: feat(client): replace jersey with okhttp in hugegraph-client (Draft PR)
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on PR #508:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/508#issuecomment-1681026562
> > Overall looks good, but we need fix ci first and improve codestyle refer to [apache/incubator-hugegraph@`master`/hugegraph-style.xml](https://github.com/apache/incubator-hugegraph/blob/master/hugegraph-style.xml?rgh-link-date=2023-08-16T17%3A32%3A51Z)
>
> 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](https://github.com/apache/incubator-hugegraph-toolchain/blob/master/.licenserc.yaml#L54) if we really don't need or can't add license header in it (also check `maven-rat` in pom.xml)
<img width="1108" alt="image" src="https://github.com/apache/incubator-hugegraph-toolchain/assets/17706099/9486864c-9328-4736-adae-74cce5055035">
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-toolchain] simon824 commented on pull request #508: feat(client): replace jersey with okhttp in hugegraph-client (Draft PR)
Posted by "simon824 (via GitHub)" <gi...@apache.org>.
simon824 commented on PR #508:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/508#issuecomment-1705920625
LGTM, @zhenyuT please resolve conflicts when you are free.
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-toolchain] javeme commented on a diff in pull request #508: feat(client): replace jersey with okhttp in hugegraph-client (Draft PR)
Posted by "javeme (via GitHub)" <gi...@apache.org>.
javeme commented on code in PR #508:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/508#discussion_r1320086105
##########
hugegraph-client/src/main/java/org/apache/hugegraph/api/graph/EdgeAPI.java:
##########
@@ -68,29 +68,30 @@ public List<String> create(List<Edge> edges, boolean checkVertex) {
public List<Edge> update(BatchEdgeRequest request) {
this.client.checkApiVersion("0.45", "batch property update");
- MultivaluedHashMap<String, Object> headers = new MultivaluedHashMap<>();
- 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);
+ Headers headers = new Headers.Builder().add("Content-Encoding", BATCH_ENCODING).build();
Review Comment:
define a const?
##########
hugegraph-client/src/main/java/org/apache/hugegraph/api/graph/EdgeAPI.java:
##########
@@ -46,16 +45,17 @@ protected String type() {
}
public Edge create(Edge edge) {
- RestResult result = this.client.post(this.path(), edge);
+ OkhttpRestResult result = this.client.post(this.path(), edge);
return result.readObject(Edge.class);
}
public List<String> create(List<Edge> edges, boolean checkVertex) {
- MultivaluedHashMap<String, Object> headers = new MultivaluedHashMap<>();
- headers.putSingle("Content-Encoding", BATCH_ENCODING);
+// MultivaluedHashMap<String, Object> headers = new MultivaluedHashMap<>();
+// headers.putSingle("Content-Encoding", BATCH_ENCODING);
Review Comment:
ditto
##########
hugegraph-client/src/main/java/org/apache/hugegraph/api/auth/UserAPI.java:
##########
@@ -40,32 +40,33 @@ protected String type() {
}
public User create(User user) {
- RestResult result = this.client.post(this.path(), user);
+ OkhttpRestResult result = this.client.post(this.path(), user);
return result.readObject(User.class);
}
public User get(Object id) {
- RestResult result = this.client.get(this.path(), formatEntityId(id));
+ OkhttpRestResult result = this.client.get(this.path(), formatEntityId(id));
return result.readObject(User.class);
}
public UserRole getUserRole(Object id) {
- String idEncoded = RestClient.encode(formatEntityId(id));
+// String idEncoded = RestClient.encode(formatEntityId(id));
Review Comment:
can we remove the unused code
##########
hugegraph-client/src/main/java/org/apache/hugegraph/rest/OkhttpRestResult.java:
##########
@@ -0,0 +1,118 @@
+/*
+ * 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 com.fasterxml.jackson.databind.JavaType;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.Module;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import lombok.SneakyThrows;
+import okhttp3.Headers;
+import okhttp3.Response;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+
+public class OkhttpRestResult {
Review Comment:
don't need to Okhttp prefix
##########
hugegraph-client/src/main/java/org/apache/hugegraph/rest/OkhttpRestClient.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * 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 okhttp3.Headers;
+
+import java.util.Map;
+
+public interface OkhttpRestClient {
Review Comment:
don't need to Okhttp prefix, we expect to keep the same interface name
##########
hugegraph-client/src/main/java/org/apache/hugegraph/api/graph/EdgeAPI.java:
##########
@@ -68,29 +68,30 @@ public List<String> create(List<Edge> edges, boolean checkVertex) {
public List<Edge> update(BatchEdgeRequest request) {
this.client.checkApiVersion("0.45", "batch property update");
- MultivaluedHashMap<String, Object> headers = new MultivaluedHashMap<>();
- 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);
Review Comment:
ditto
##########
hugegraph-client/src/main/java/org/apache/hugegraph/rest/OkhttpAbstractRestClient.java:
##########
@@ -0,0 +1,449 @@
+/*
+ * 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 com.google.common.collect.ImmutableMap;
+import lombok.SneakyThrows;
+import okhttp3.*;
+import okio.BufferedSink;
+import okio.GzipSink;
+import okio.Okio;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.hugegraph.util.JsonUtil;
+
+import javax.net.ssl.*;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.net.URI;
+import java.security.KeyStore;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+
+public abstract class OkhttpAbstractRestClient implements OkhttpRestClient {
+
+ private OkHttpClient client;
+
+ private String baseUrl;
+
+ public OkhttpAbstractRestClient(String url, int timeout) {
+ this(url, OkhttpConfig.builder()
+ .timeout(timeout)
+ .build());
+ }
+
+ public OkhttpAbstractRestClient(String url, String user, String password,
+ Integer timeout) {
+ this(url, OkhttpConfig.builder()
+ .user(user).password(password)
+ .timeout(timeout)
+ .build());
+ }
+
+ public OkhttpAbstractRestClient(String url, int timeout,
+ int maxTotal, int maxPerRoute) {
+ this(url, null, null, timeout, maxTotal, maxPerRoute);
+ }
+
+ public OkhttpAbstractRestClient(String url, int timeout, int idleTime,
+ int maxTotal, int maxPerRoute) {
+ this(url, OkhttpConfig.builder()
+ .idleTime(idleTime)
+ .timeout(timeout)
+ .maxTotal(maxTotal)
+ .maxPerRoute(maxPerRoute)
+ .build());
+ }
+
+ public OkhttpAbstractRestClient(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());
+ }
+
+ public OkhttpAbstractRestClient(String url, String user, String password,
+ int timeout, int maxTotal, int maxPerRoute,
+ String trustStoreFile,
+ String trustStorePassword) {
+ this(url, OkhttpConfig.builder()
+ .user(user).password(password)
+ .timeout(timeout)
+ .maxTotal(maxTotal)
+ .maxPerRoute(maxPerRoute)
+ .trustStoreFile(trustStoreFile)
+ .trustStorePassword(trustStorePassword)
+ .build());
+ }
+
+ public OkhttpAbstractRestClient(String url, String token, Integer timeout) {
+ this(url, OkhttpConfig.builder()
+ .token(token)
+ .timeout(timeout)
+ .build());
+ }
+
+ public OkhttpAbstractRestClient(String url, String token, Integer timeout,
+ Integer maxTotal, Integer maxPerRoute) {
+ this(url,OkhttpConfig.builder()
+ .token(token)
+ .timeout(timeout)
+ .maxTotal(maxTotal)
+ .maxPerRoute(maxPerRoute)
+ .build());
+ }
+
+ public OkhttpAbstractRestClient(String url, String token, Integer timeout,
+ Integer maxTotal, Integer maxPerRoute,
+ String trustStoreFile,
+ String trustStorePassword) {
+ this(url,OkhttpConfig.builder()
+ .token(token)
+ .timeout(timeout)
+ .maxTotal(maxTotal)
+ .maxPerRoute(maxPerRoute)
+ .trustStoreFile(trustStoreFile)
+ .trustStorePassword(trustStorePassword)
+ .build());
+ }
+
+ public OkhttpAbstractRestClient(String url, OkhttpConfig okhttpConfig) {
+ this.baseUrl = url;
+ this.client = getOkhttpClient(okhttpConfig);
+ }
+
+ private OkHttpClient getOkhttpClient(OkhttpConfig okhttpConfig) {
+ OkHttpClient.Builder builder = new OkHttpClient.Builder();
+
+ if(okhttpConfig.getTimeout()!=null) {
+ builder.connectTimeout(okhttpConfig.getTimeout(), TimeUnit.MILLISECONDS)
+ .readTimeout(okhttpConfig.getTimeout(), TimeUnit.MILLISECONDS);
+ }
+
+ if(okhttpConfig.getIdleTime()!=null) {
+ ConnectionPool connectionPool = new ConnectionPool(5, okhttpConfig.getIdleTime(), TimeUnit.MILLISECONDS);
+ builder.connectionPool(connectionPool);
+ }
+
+
+ //auth
Review Comment:
can we add more details for comments
##########
pom.xml:
##########
@@ -151,17 +151,35 @@
<!-- submodules will not inherit the dependencies unless they address it -->
<dependencyManagement>
<dependencies>
- <dependency>
- <groupId>org.glassfish.jersey</groupId>
- <artifactId>jersey-bom</artifactId>
- <version>${jersey.version}</version>
- <type>pom</type>
- <scope>import</scope>
- </dependency>
+<!-- <dependency>-->
+<!-- <groupId>org.glassfish.jersey</groupId>-->
+<!-- <artifactId>jersey-bom</artifactId>-->
+<!-- <version>${jersey.version}</version>-->
+<!-- <type>pom</type>-->
+<!-- <scope>import</scope>-->
+<!-- </dependency>-->
Review Comment:
we can remove all the unused code
##########
hugegraph-client/src/main/java/org/apache/hugegraph/api/graph/VertexAPI.java:
##########
@@ -68,19 +66,21 @@ public List<Object> create(List<Vertex> vertices) {
public List<Vertex> update(BatchVertexRequest request) {
this.client.checkApiVersion("0.45", "batch property update");
- MultivaluedHashMap<String, Object> headers = new MultivaluedHashMap<>();
- 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);
+ Headers headers = new Headers.Builder().add("Content-Encoding", BATCH_ENCODING).build();
+ OkhttpRestResult result = this.client.put(this.batchPath(), null,
request, headers);
return result.readList(this.type(), Vertex.class);
}
public int update(BatchOlapPropertyRequest request) {
this.client.checkApiVersion("0.59", "olap property batch update");
- MultivaluedHashMap<String, Object> headers = new MultivaluedHashMap<>();
- headers.putSingle("Content-Encoding", BATCH_ENCODING);
+// MultivaluedHashMap<String, Object> headers = new MultivaluedHashMap<>();
+// headers.putSingle("Content-Encoding", BATCH_ENCODING);
Review Comment:
ditto
##########
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;
Review Comment:
ok, I mean we can try to import okhttp3.Response
##########
hugegraph-client/src/main/java/org/apache/hugegraph/rest/OkhttpConfig.java:
##########
@@ -0,0 +1,37 @@
+/*
+ * 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 lombok.Builder;
+import lombok.Getter;
+import lombok.Setter;
+
+@Builder
+@Getter
+@Setter
+public class OkhttpConfig {
Review Comment:
expect a blank line after the class define
##########
hugegraph-client/src/main/java/org/apache/hugegraph/driver/HugeClient.java:
##########
@@ -60,7 +60,8 @@ public HugeClient(HugeClientBuilder builder) {
builder.maxConnsPerRoute(),
builder.trustStoreFile(),
builder.trustStorePassword());
- } catch (ProcessingException e) {
+ } catch (Exception e) {
+ log.error("", e);
Review Comment:
please improve the log message
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-toolchain] imbajin commented on a diff in pull request #508: feat(client): replace jersey with okhttp in hugegraph-client (Draft PR)
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #508:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/508#discussion_r1296264741
##########
hugegraph-client/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker:
##########
@@ -0,0 +1 @@
+mock-maker-inline
Review Comment:
And is this file necessary? (maybe we could try to add license header first)
```suggestion
# license header in shell
# ....
# end
mock-maker-inline
```
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-toolchain] zhenyuT commented on pull request #508: feat(client): replace jersey with okhttp in hugegraph-client (Draft PR)
Posted by "zhenyuT (via GitHub)" <gi...@apache.org>.
zhenyuT commented on PR #508:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/508#issuecomment-1682442395
> > 麻烦先看下代码，如果大体方向没问题的话，我再继续进行代码优化 (以及调整 hugegrpah-common 中的依赖, 目前是直接在 client 调整)
>
> 整体方案没问题的。 CI失败，看了下是超时问题SocketTimeoutException，请确保各类超时时间设置正确了：
>
> ```
> MultiNodeShortestPathApiTest Time elapsed: 10.038 s <<< ERROR!
> java.net.SocketTimeoutException: timeout
> ```
有点奇怪，我fork到自己仓库，执行是成功的，没有遇到超时的问题。两边执行是有什么区别吗？
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-toolchain] z7658329 commented on pull request #508: feat(client): replace jersey with okhttp in hugegraph-client (Draft PR)
Posted by "z7658329 (via GitHub)" <gi...@apache.org>.
z7658329 commented on PR #508:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/508#issuecomment-1689165155
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](https://github.com/apache/incubator-hugegraph-toolchain/issues/495) @zhenyuT
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-toolchain] imbajin commented on a diff in pull request #508: feat(client): replace jersey with okhttp in hugegraph-client (Draft PR)
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #508:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/508#discussion_r1310029258
##########
hugegraph-client/pom.xml:
##########
@@ -37,28 +37,73 @@
</description>
<properties>
+ <lombok.version>1.18.8</lombok.version>
+ <okhttp.version>4.10.0</okhttp.version>
Review Comment:
> 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?)
##########
hugegraph-client/pom.xml:
##########
@@ -37,28 +37,73 @@
</description>
<properties>
+ <lombok.version>1.18.8</lombok.version>
+ <okhttp.version>4.10.0</okhttp.version>
Review Comment:
> 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?)
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-toolchain] simon824 commented on a diff in pull request #508: feat(client): replace jersey with okhttp in hugegraph-client (Draft PR)
Posted by "simon824 (via GitHub)" <gi...@apache.org>.
simon824 commented on code in PR #508:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/508#discussion_r1294299014
##########
hugegraph-client/src/main/java/org/apache/hugegraph/rest/TokenInterceptor.java:
##########
@@ -0,0 +1,27 @@
+package org.apache.hugegraph.rest;
Review Comment:
ditto
##########
hugegraph-client/src/main/java/org/apache/hugegraph/rest/OkhttpConfig.java:
##########
@@ -0,0 +1,20 @@
+package org.apache.hugegraph.rest;
Review Comment:
ditto
##########
hugegraph-client/src/main/java/org/apache/hugegraph/rest/BasicAuthInterceptor.java:
##########
@@ -0,0 +1,28 @@
+package org.apache.hugegraph.rest;
Review Comment:
Add license header
##########
hugegraph-client/src/test/java/org/apache/hugegraph/BaseClientTest.java:
##########
@@ -49,7 +49,8 @@
public class BaseClientTest {
- protected static final String BASE_URL = "http://127.0.0.1:8080";
+// protected static final String BASE_URL = "http://127.0.0.1:8080";
+ protected static final String BASE_URL = "http://172.19.102.150:8080";
Review Comment:
Don't commit these changes
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-toolchain] imbajin commented on a diff in pull request #508: feat(client): replace jersey with okhttp in hugegraph-client (Draft PR)
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #508:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/508#discussion_r1296264741
##########
hugegraph-client/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker:
##########
@@ -0,0 +1 @@
+mock-maker-inline
Review Comment:
And is this file necessary? (maybe we could try to add license header first)
```suggestion
# license header with `#` comment
# ....
# end license
mock-maker-inline
```
--
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: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
[GitHub] [incubator-hugegraph-toolchain] javeme commented on pull request #508: feat(client): replace jersey with okhttp in hugegraph-client (Draft PR)
Posted by "javeme (via GitHub)" <gi...@apache.org>.
javeme commented on PR #508:
URL: https://github.com/apache/incubator-hugegraph-toolchain/pull/508#issuecomment-1682255984
> 麻烦先看下代码，如果大体方向没问题的话，我再继续进行代码优化 (以及调整 hugegrpah-common 中的依赖, 目前是直接在 client 调整)
整体方案没问题的。
CI失败，看了下是超时问题SocketTimeoutException，请确保各类超时时间设置正确了：
```
MultiNodeShortestPathApiTest Time elapsed: 10.038 s <<< ERROR!
java.net.SocketTimeoutException: 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.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org