Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(common): replace jersey dependencies with OkHttp (Breaking Change) #133

Merged
merged 32 commits into from Nov 22, 2023

Conversation

zhenyuT
Copy link
Contributor

@zhenyuT zhenyuT commented Aug 27, 2023

之前在tool-chains提过一个pr,直接修改client的代码将jersey使用okhttp替换

现在尝试直接修改common的代码,不过common的代码中单元测试和jersey是强相关的,在这一块遇到一些问题,然后尝试自己修改单测代码进行跑通。不过由于对单测的代码不太熟悉,对于代码的意图不太了解,麻烦社区同学帮忙看看

Breaking Changes

  • org.apache.http.HttpStatus is removed now, replace it to java.net.HttpURLConnection (JDK bundled)
  • jakarta.ws.rs.core.MultivaluedHashMap is removed now, replace it to org.apache.hugegraph.rest.RestHeaders
       MultivaluedHashMap<String, Object> headers = new MultivaluedHashMap<>();
       headers.putSingle("Content-Encoding", BATCH_ENCODING);
    For example,replace the upon code to:
     RestHeaders headers = new RestHeaders().add("Content-Encoding", BATCH_ENCODING);
  • jakarta.ws.rs.core.Response is removed now, replace it to okhttp3.Response.
  • org.glassfish.jersey.uri.UriComponent is removed, just not use it. There's no need to encode the query params manual, just remove the code UriComponent.encode(raw, Type.QUERY_PARAM_SPACE_ENCODED).

Other repo/users could refer the info to update/adapt the new version, and comment in this PR if anything missed

@codecov
Copy link

codecov bot commented Aug 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b49be05) 93.15% compared to head (81aa7ca) 93.15%.

Additional details and impacted files
@@            Coverage Diff            @@
##             master     #133   +/-   ##
=========================================
  Coverage     93.15%   93.15%           
  Complexity       65       65           
=========================================
  Files             9        9           
  Lines           263      263           
  Branches         22       22           
=========================================
  Hits            245      245           
  Misses            8        8           
  Partials         10       10           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions
Copy link

github-actions bot commented Oct 3, 2023

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

@imbajin imbajin requested a review from javeme October 31, 2023 08:57
@imbajin imbajin added this to the 1.2.0 milestone Oct 31, 2023
private Integer idleTime = 5;
private TimeUnit idleTimeUnit = TimeUnit.MINUTES;
private Integer idleTime = 30;
private TimeUnit idleTimeUnit = TimeUnit.SECONDS;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems don't need to add a var for the unit, prefer to just hardcode it

@@ -77,4 +77,18 @@ public boolean equals(Object obj) {
public okhttp3.Headers toOkHttpHeader() {
return this.headersBuilder.build();
}

public static class HttpHeadersConstant {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer to merge these vars into class RestHeaders

@@ -184,7 +186,7 @@ private OkHttpClient buildOkHttpClient(RestClientConfig config) {
if (config.getMaxIdleConns() != null || config.getIdleTime() != null) {
ConnectionPool connectionPool = new ConnectionPool(config.getMaxIdleConns(),
config.getIdleTime(),
TimeUnit.MINUTES);
config.getIdleTimeUnit());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can just hardcode TimeUnit.SECONDS here

Copy link
Contributor

@javeme javeme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@imbajin imbajin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks a lot for the job, I'll publish it to stage maven repo soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants