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

[KYUUBI #6294] Simplify Netty and gRPC dependency management #6310

Closed
wants to merge 3 commits into from

Conversation

PorterZhang2021
Copy link
Contributor

@PorterZhang2021 PorterZhang2021 commented Apr 15, 2024

🔍 Description

Issue References 🔗

This pull request fixes #6294

Describe Your Solution 🔧

The netty jars come from grpc-netty, arrow-memory-netty, use netty-bom and grpc-bom to simplify the dependency management.

Run ./build/dependency.sh --replace

Types of changes 🔖

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Test Plan 🧪

Behavior Without This Pull Request ⚰️

Behavior With This Pull Request 🎉

Related Unit Tests


Checklist 📝

Be nice. Be informative.

@PorterZhang2021
Copy link
Contributor Author

Hello @pan3793, Mr.Pan, I have attempted to remove the content of io.nettyand have also tried packaging and testing again, but I have not actually conducted startup testing.
For other unused jars, after using mvn dependency: analyze, other pom.xml showed unused declared dependencies found, as mentioned below:
image
Because I am not sure about the impact of deletion and do not know how to test the impact, I did not delete it
If you have more detailed guidance, I may give it a try, thank you.

<artifactId>netty-all</artifactId>
<version>${netty.version}</version>
<exclusions>
<exclusion>
Copy link
Member

Choose a reason for hiding this comment

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

actually, my vision is to extend this exclusion list instead of declaring everything explicitly. for example, netty-codec-socks does not used by Kyuubi, and we should add it into exclusion list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

emmm, I know, I will try it again. Thanks

Copy link
Member

Choose a reason for hiding this comment

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

I just noticed I was wrong on this assumption, apologize for this comment.

The netty jars come from grpc-netty, arrow-memory-netty, use netty-bom and grpc-bom to simplify the dependency management.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, thanks.

@codecov-commenter
Copy link

codecov-commenter commented Apr 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.45%. Comparing base (9086cfe) to head (3270301).
Report is 5 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #6310      +/-   ##
============================================
- Coverage     58.47%   58.45%   -0.02%     
  Complexity       24       24              
============================================
  Files           652      653       +1     
  Lines         39645    39775     +130     
  Branches       5454     5473      +19     
============================================
+ Hits          23181    23252      +71     
- Misses        13984    14034      +50     
- Partials       2480     2489       +9     

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

@PorterZhang2021
Copy link
Contributor Author

Hello, Mr.Pan @pan3793 , I try it again, and exclusion everyone, I removed all dependencies and tried to start it without any issues.
But for other related dependencies, I don't quite understand how to remove them. I did the following:

  1. Use mvn dependency: analyze (using kyuubi_ha as an example)
    image
  2. Use mvn dependency: tree
    image
  3. open kyuubi_ha -> pom.xml, then delete io.netty:netty-transport-native-epoll:jar:linux-aarch_64, io.netty:netty-transport-native-epoll:jar:linux-x86_64
  4. then repackages
    I'm not sure if my example is accurate or not. If it's not, can you provide me with some relevant reference materials? I've searched a lot online but I still don't quite understand this aspect.

@pan3793
Copy link
Member

pan3793 commented Apr 17, 2024

seems a little bit more complex than I originally thought, I will take a look.

@pan3793 pan3793 changed the title [KYUUBI #6294] Prune unused Netty libraries [KYUUBI #6294] Simplify Netty and gRPC dependency management Apr 17, 2024
@pan3793 pan3793 closed this in 962de72 Apr 17, 2024
pan3793 added a commit that referenced this pull request Apr 17, 2024
# 🔍 Description
## Issue References 🔗

This pull request fixes #6294

## Describe Your Solution 🔧

The netty jars come from `grpc-netty`, `arrow-memory-netty`, use `netty-bom` and `grpc-bom` to simplify the dependency management.

Run `./build/dependency.sh --replace`

## Types of changes 🔖

- [ ] Bugfix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Test Plan 🧪

#### Behavior Without This Pull Request ⚰️

#### Behavior With This Pull Request 🎉

#### Related Unit Tests

---

# Checklist 📝

- [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html)

**Be nice. Be informative.**

Closes #6310 from PorterZhang2021/issue-6294.

Closes #6294

3270301 [Cheng Pan] simplify netty dependency management
cdb435d [PorterZhang2021] [# 6294] Prune unused Netty libraries
e0676ed [PorterZhang2021] Finished Prune unused Netty libraries [#6294]

Lead-authored-by: PorterZhang2021 <PorterZhang2021@outlook.com>
Co-authored-by: Cheng Pan <chengpan@apache.org>
Signed-off-by: Cheng Pan <chengpan@apache.org>
(cherry picked from commit 962de72)
Signed-off-by: Cheng Pan <chengpan@apache.org>
@pan3793 pan3793 added this to the v1.9.1 milestone Apr 17, 2024
@pan3793
Copy link
Member

pan3793 commented Apr 17, 2024

Merged to master/1.9.1

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

Successfully merging this pull request may close these issues.

[TASK][EASY] Simplify Netty and gRPC dependency management
3 participants