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

[#343] improvement(build): Shade Netty Native lib #924

Merged
merged 5 commits into from
Jun 10, 2023

Conversation

cchung100m
Copy link
Collaborator

Hi @jerqi

Any comments and suggestions would be appreciated if you are available, thank you.

What changes were proposed in this pull request?

Add the shade Netty Native lib into pom.xml

Why are the changes needed?

Fix: #343

Does this PR introduce any user-facing change?

No.

How was this patch tested?

current UT

@cchung100m cchung100m changed the title [apache#343][Improvement] Shade Netty Native lib [#343][Improvement] Shade Netty Native lib Jun 3, 2023
@cchung100m cchung100m changed the title [#343][Improvement] Shade Netty Native lib [#343]improvement(pom.xml): Shade Netty Native lib Jun 3, 2023
@jerqi jerqi changed the title [#343]improvement(pom.xml): Shade Netty Native lib [#343]improvement(build): Shade Netty Native lib Jun 3, 2023
@jerqi
Copy link
Contributor

jerqi commented Jun 3, 2023

We should only shade jars in the framework client like client-spark3, client-spark2, client-mr. You can modify

<id>rss-client-spark3-jar</id>

@jerqi jerqi changed the title [#343]improvement(build): Shade Netty Native lib [#343] improvement(build): Shade Netty Native lib Jun 3, 2023
@cchung100m
Copy link
Collaborator Author

We should only shade jars in the framework client like client-spark3, client-spark2, client-mr. You can modify

<id>rss-client-spark3-jar</id>

Hi @jerqi

Thanks for the prompt reply. I'd like to confirm whether the proposed change is feasible or not.

@jerqi
Copy link
Contributor

jerqi commented Jun 4, 2023

We should only shade jars in the framework client like client-spark3, client-spark2, client-mr. You can modify

<id>rss-client-spark3-jar</id>

Hi @jerqi

Thanks for the prompt reply. I'd like to confirm whether the proposed change is feasible or not.

Yes, I think it is feasible. Could you also modify the client-mr?

@codecov-commenter
Copy link

codecov-commenter commented Jun 4, 2023

Codecov Report

Merging #924 (cefe9fb) into master (b6a725b) will increase coverage by 1.81%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master     #924      +/-   ##
============================================
+ Coverage     55.29%   57.10%   +1.81%     
- Complexity     2273     2274       +1     
============================================
  Files           344      324      -20     
  Lines         16983    14623    -2360     
  Branches       1349     1349              
============================================
- Hits           9391     8351    -1040     
+ Misses         7048     5798    -1250     
+ Partials        544      474      -70     

see 23 files with indirect coverage changes

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

@cchung100m cchung100m force-pushed the feat_343_Shade_Netty_Native_lib branch from 5f219f4 to 80c1fa1 Compare June 8, 2023 14:42
@cchung100m
Copy link
Collaborator Author

Hi @jerqi

It seems like there are some integration testing errors that occurred, any suggestions would be appreciated if you are available to help.

server/pom.xml Outdated
@@ -203,6 +203,13 @@
<pattern>com.fasterxml.jackson.databind</pattern>
<shadedPattern>${rss.shade.packageName}.jackson.databind</shadedPattern>
</relocation>
<relocation>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you revert this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @jerqi Thanks for the kind reminder and suggestions. I updated the part you mentioned.

@cchung100m cchung100m requested a review from jerqi June 10, 2023 03:14
Copy link
Contributor

@jerqi jerqi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @cchung100m , merged to master.

@jerqi jerqi merged commit 84167c5 into apache:master Jun 10, 2023
@cchung100m cchung100m deleted the feat_343_Shade_Netty_Native_lib branch June 10, 2023 04:26
jerqi pushed a commit that referenced this pull request Jan 2, 2024
…1410)

### What changes were proposed in this pull request?

Fix a bug that was introduced from [#343](#343).
The Netty native libraries have never been successfully shaded. PR [#924](#924) is not working at all.

### Why are the changes needed?

For [#1409](#1409)

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

By running the RSS Netty client with Epoll mode successfully.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement] Shade Netty Native lib
3 participants