[#864] feat(server): Introduce Jersey to strengthen REST API #939

xianjingfeng · 2023-06-08T07:16:17Z

What changes were proposed in this pull request?

Introduce Jersey to strengthen REST API.
To use a new version of Jersey and being compatible with Hadoop 2.x, we have to use a shaded version of Jersery.
However Uniffle doesn't do binary release currently, we choose to introduce hbase's one as a temporary solution.

todo: release a shaded Jersey package for Uniffle's internal use or migrate to Graddle.

Why are the changes needed?

Fix: #864

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Added UT.

# Conflicts: # coordinator/src/main/java/org/apache/uniffle/coordinator/CoordinatorServer.java

xianjingfeng · 2023-06-08T07:23:00Z

Jersey1 is used in this PR. I have try to use jersey2, but i found it is not compatible with hadoop.
We can't use lambda expression in jersey resources with jersey 1.9. https://stackoverflow.com/questions/24232784/jersey-and-java-8-lambda-expression

codecov-commenter · 2023-06-08T07:38:45Z

Codecov Report

Merging #939 (1bf8ec2) into master (66c752f) will decrease coverage by 4.26%.
The diff coverage is 52.26%.

@@             Coverage Diff              @@
##             master     #939      +/-   ##
============================================
- Coverage     58.54%   54.28%   -4.26%     
- Complexity     1611     2500     +889     
============================================
  Files           195      357     +162     
  Lines         11035    18153    +7118     
  Branches        966     1758     +792     
============================================
+ Hits           6460     9855    +3395     
- Misses         4195     7683    +3488     
- Partials        380      615     +235

Impacted Files	Coverage Δ
...a/org/apache/uniffle/client/HttpClientFactory.java	`0.00% <0.00%> (ø)`
...java/org/apache/uniffle/client/RestClientConf.java	`0.00% <0.00%> (ø)`
...java/org/apache/uniffle/client/RestClientImpl.java	`0.00% <0.00%> (ø)`
...a/org/apache/uniffle/client/UniffleRestClient.java	`0.00% <0.00%> (ø)`
...uniffle/client/exception/UniffleRestException.java	`0.00% <0.00%> (ø)`
...rg/apache/hadoop/mapred/RssMapOutputCollector.java	`0.00% <0.00%> (ø)`
...java/org/apache/hadoop/mapred/SortWriteBuffer.java	`90.90% <ø> (ø)`
...n/java/org/apache/hadoop/mapreduce/MRIdHelper.java	`0.00% <ø> (ø)`
.../java/org/apache/hadoop/mapreduce/RssMRConfig.java	`23.07% <ø> (ø)`
...n/java/org/apache/hadoop/mapreduce/RssMRUtils.java	`47.82% <0.00%> (ø)`
... and 147 more

... and 164 files with indirect coverage changes

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

jerqi · 2023-06-10T05:57:40Z

SGTM, Let's merge this after #768 #937.

advancedxy · 2023-06-14T14:29:02Z

I'm kind of busy these days. Let me review this tomorrow.

advancedxy

Generally lgtm.

common/src/main/java/org/apache/uniffle/common/web/JettyServer.java

common/pom.xml

common/src/main/java/org/apache/uniffle/common/web/resource/PrometheusMetricResource.java

advancedxy · 2023-06-15T06:16:50Z

We can't use lambda expression in jersey resources with jersey 1.9. https://stackoverflow.com/questions/24232784/jersey-and-java-8-lambda-expression

Is it possible to upgrade Jersey to at least 1.9+? Otherwise, new code may introduce lambda, and that would be crushed in runtime...

xianjingfeng · 2023-06-15T07:12:54Z

We can't use lambda expression in jersey resources with jersey 1.9. https://stackoverflow.com/questions/24232784/jersey-and-java-8-lambda-expression

Is it possible to upgrade Jersey to at least 1.9+? Otherwise, new code may introduce lambda, and that would be crushed in runtime...

I will try it.

…ory leak (apache#773)" This reverts commit 89c2b92.

…apache#773) 1. To avoid memory leak by caching of proxy user UGI. Fix: apache#772 The Hadoop filesystem instance will be created too many time in cache, which will cause the shuffle server memory leak. As we know, the filesystem cache's key is built by the scheme、authority and UGI. The scheme and authority are not changed every time. But for UGI, if we invoke the createProxyUser, it will always create a new one, that means the every invoking `Filesystem.get()`, it will be cached due to different key. No. 1. Existing UTs 2. Added tests

…ory leak (apache#773)" This reverts commit 42d9b0a.

… leak (apache#773) (apache#824) 1. To avoid memory leak by caching of proxy user UGI. Fix: apache#772 The Hadoop filesystem instance will be created too many time in cache, which will cause the shuffle server memory leak. As we know, the filesystem cache's key is built by the scheme、authority and UGI. The scheme and authority are not changed every time. But for UGI, if we invoke the createProxyUser, it will always create a new one, that means the every invoking `Filesystem.get()`, it will be cached due to different key. No. 1. Existing UTs 2. Added tests

…ion when rss.storage.type without MEMORY. (apache#887)" This reverts commit 4423b43.

xianjingfeng · 2023-06-30T09:38:41Z

Already changed to use hbase-thirdparty. PTAL @jerqi @advancedxy @yl09099

yl09099 · 2023-06-30T09:46:53Z

Already changed to use hbase-thirdparty. PTAL @jerqi @advancedxy @yl09099

I also tried to solve it, but I introduced the hbase-shaded-jackson-jaxrs-json-provider package, you can not write the JsonConverter conversion class, I have resolved the conflict in #960, you see my way is okay?

xianjingfeng · 2023-06-30T10:09:02Z

Already changed to use hbase-thirdparty. PTAL @jerqi @advancedxy @yl09099

Already changed to use hbase-thirdparty. PTAL @jerqi @advancedxy @yl09099

I also tried to solve it, but I introduced the hbase-shaded-jackson-jaxrs-json-provider package, you can not write the JsonConverter conversion class, I have resolved the conflict in #960, you see my way is okay?

I tried to register resource in the same way as you before, but I found that I couldn’t get HttpServletRequest or HttpServletRequest, have you tested it and it’s ok?

This reverts commit 621977f.

advancedxy · 2023-07-03T07:28:08Z

Hi @xianjingfeng and @slfan1989 this and #963 which one should be merged first? And by the way, seems like these two PRs both have PR failrures.

xianjingfeng · 2023-07-03T08:03:32Z

Hi @xianjingfeng and @slfan1989 this and #963 which one should be merged first?

It doesn't matter which one is merged first. But #963 should not contain the content that introduces jersey.

And by the way, seems like these two PRs both have PR failrures.

The failrure of this pr because of flaky test.

slfan1989 · 2023-07-03T11:08:19Z

Hi @xianjingfeng and @slfan1989 this and #963 which one should be merged first? And by the way, seems like these two PRs both have PR failrures.

@advancedxy @xianjingfeng From my personal point of view, this pr should be merged first. #963 is still under discussion, and yl09099 is currently working on the documentation. It might take some time before we can merge it.

yl09099 · 2023-07-03T11:14:05Z

The failrure of this pr because of flaky test.

This PR should be merged first

advancedxy

Generally lgtm, left two minor comments

common/src/main/java/org/apache/uniffle/common/web/resource/PrometheusMetricResource.java

common/src/main/java/org/apache/uniffle/common/web/JerseyAutoDiscoverable.java

advancedxy · 2023-07-06T14:59:24Z

@xianjingfeng This looks good to me now. I modified some content of the PR summary. Please amend/update accordingly if there are anything missing or left. Then I think we can squash and merge this PR.

xianjingfeng · 2023-07-07T01:55:09Z

@xianjingfeng This looks good to me now. I modified some content of the PR summary. Please amend/update accordingly if there are anything missing or left. Then I think we can squash and merge this PR.

SGTM. Thanks for your work

xianjingfeng · 2023-07-07T01:55:47Z

Do you have any other suggestions? @jerqi @yl09099

slfan1989 · 2023-07-07T02:26:42Z

Do you have any other suggestions? @jerqi @yl09099

LGTM +1.

yl09099 · 2023-07-07T02:30:05Z

Do you have any other suggestions? @jerqi @yl09099

LGTM +1.

xianjingfeng · 2023-07-07T16:11:34Z

Merged. Thanks all.

xianjingfeng added 12 commits December 13, 2022 19:26

Support metric reporter and Support promethues push gateway

5a4e760

Merge branch 'master' into issue_169

b7828bf

# Conflicts: # coordinator/src/main/java/org/apache/uniffle/coordinator/CoordinatorServer.java

Optimize

4226a0e

Remove useless modification

02c5161

Miss files

583db4f

Fix code style

52eaab5

Optimize

480d4de

Add instanceId

6f76966

合并metric接口

c4293e0

ISSUE-455

dd0ee24

ISSUE-456

39bc96c

ISSUE-468

9e541e2

xianjingfeng requested review from jerqi, advancedxy and zuston June 8, 2023 07:23

xianjingfeng closed this Jun 8, 2023

xianjingfeng reopened this Jun 8, 2023

advancedxy reviewed Jun 15, 2023

View reviewed changes

common/src/main/java/org/apache/uniffle/common/web/JettyServer.java Outdated Show resolved Hide resolved

common/pom.xml Outdated Show resolved Hide resolved

common/src/main/java/org/apache/uniffle/common/web/resource/PrometheusMetricResource.java Outdated Show resolved Hide resolved

xianjingfeng mentioned this pull request Jun 18, 2023

[#768] feat(cli): Cli method for blacklist update #931

Merged

jerqi and others added 5 commits June 20, 2023 14:33

Revert "[apache#772] fix(kerberos): cache proxy user ugi to avoid mem…

e36e3a3

…ory leak (apache#773)" This reverts commit 89c2b92.

Revert "[apache#772] fix(kerberos): cache proxy user ugi to avoid mem…

4cdfbec

…ory leak (apache#773)" This reverts commit 42d9b0a.

Revert "[apache#886] fix(mr): MR Client may lost data or throw except…

4b82788

…ion when rss.storage.type without MEMORY. (apache#887)" This reverts commit 4423b43.

xianjingfeng added 3 commits June 30, 2023 16:44

use hbase thirdparty

f6e08af

nit

3c7a5bb

fix spotbugs

d62081e

fix bug

7c7f8a0

xianjingfeng added 3 commits June 30, 2023 18:13

fix codestyle

d650f00

introduce hbase-shaded-jackson-jaxrs-json-provider

621977f

Revert "introduce hbase-shaded-jackson-jaxrs-json-provider"

1bf8ec2

This reverts commit 621977f.

advancedxy self-assigned this Jul 4, 2023

jerqi unassigned advancedxy Jul 4, 2023

jerqi requested a review from advancedxy July 4, 2023 11:01

advancedxy reviewed Jul 4, 2023

View reviewed changes

common/src/main/java/org/apache/uniffle/common/web/resource/PrometheusMetricResource.java Outdated Show resolved Hide resolved

common/src/main/java/org/apache/uniffle/common/web/JerseyAutoDiscoverable.java Show resolved Hide resolved

xianjingfeng added 2 commits July 6, 2023 15:19

nit

04c9d31

fix license

07f3daa

advancedxy approved these changes Jul 6, 2023

View reviewed changes

slfan1989 approved these changes Jul 7, 2023

View reviewed changes

xianjingfeng merged commit b8038cb into apache:master Jul 7, 2023

xianjingfeng deleted the issue_864_jersey1 branch August 9, 2024 06:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#864] feat(server): Introduce Jersey to strengthen REST API #939

[#864] feat(server): Introduce Jersey to strengthen REST API #939

xianjingfeng commented Jun 8, 2023 •

edited by advancedxy

Loading

xianjingfeng commented Jun 8, 2023

codecov-commenter commented Jun 8, 2023 •

edited

Loading

jerqi commented Jun 10, 2023 •

edited

Loading

advancedxy commented Jun 14, 2023

advancedxy left a comment

advancedxy commented Jun 15, 2023

xianjingfeng commented Jun 15, 2023

xianjingfeng commented Jun 30, 2023

yl09099 commented Jun 30, 2023

xianjingfeng commented Jun 30, 2023

advancedxy commented Jul 3, 2023

xianjingfeng commented Jul 3, 2023

slfan1989 commented Jul 3, 2023

yl09099 commented Jul 3, 2023

advancedxy left a comment

advancedxy commented Jul 6, 2023

xianjingfeng commented Jul 7, 2023

xianjingfeng commented Jul 7, 2023

slfan1989 commented Jul 7, 2023

yl09099 commented Jul 7, 2023

xianjingfeng commented Jul 7, 2023

[#864] feat(server): Introduce Jersey to strengthen REST API #939

[#864] feat(server): Introduce Jersey to strengthen REST API #939

Conversation

xianjingfeng commented Jun 8, 2023 • edited by advancedxy Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

xianjingfeng commented Jun 8, 2023

codecov-commenter commented Jun 8, 2023 • edited Loading

Codecov Report

jerqi commented Jun 10, 2023 • edited Loading

advancedxy commented Jun 14, 2023

advancedxy left a comment

Choose a reason for hiding this comment

advancedxy commented Jun 15, 2023

xianjingfeng commented Jun 15, 2023

xianjingfeng commented Jun 30, 2023

yl09099 commented Jun 30, 2023

xianjingfeng commented Jun 30, 2023

advancedxy commented Jul 3, 2023

xianjingfeng commented Jul 3, 2023

slfan1989 commented Jul 3, 2023

yl09099 commented Jul 3, 2023

advancedxy left a comment

Choose a reason for hiding this comment

advancedxy commented Jul 6, 2023

xianjingfeng commented Jul 7, 2023

xianjingfeng commented Jul 7, 2023

slfan1989 commented Jul 7, 2023

yl09099 commented Jul 7, 2023

xianjingfeng commented Jul 7, 2023

xianjingfeng commented Jun 8, 2023 •

edited by advancedxy

Loading

codecov-commenter commented Jun 8, 2023 •

edited

Loading

jerqi commented Jun 10, 2023 •

edited

Loading