Spark 3.3: Relocate all Netty dependencies#6107
Conversation
| // to make sure io.netty.buffer only comes from project(':iceberg-arrow') | ||
| // to make sure netty libs only come from project(':iceberg-arrow') | ||
| exclude group: 'io.netty', module: 'netty-buffer' | ||
| exclude group: 'io.netty', module: 'netty-common' |
There was a problem hiding this comment.
We exclude netty-common from many deps but not all. I feel it is safer to be consistent.
There was a problem hiding this comment.
How about enforce the version instead of excluding here and there?
There was a problem hiding this comment.
I always consider forcing as the last resort. If we force a version, can it lead to unexpected consequences in libs that would normally pull in another version?
I'd be open to consider forcing but in a separate PR as this one is fixing the relocation in the current approach.
There was a problem hiding this comment.
I don't see any problem of enforcing in this context. Either works for me, though.
| relocate 'org.apache.httpcomponents.client5', 'org.apache.iceberg.shaded.org.apache.httpcomponents.client5' | ||
| // relocate Arrow and related deps to shade Iceberg specific version | ||
| relocate 'io.netty.buffer', 'org.apache.iceberg.shaded.io.netty.buffer' | ||
| relocate 'io.netty', 'org.apache.iceberg.shaded.io.netty' |
There was a problem hiding this comment.
I think we should be specific about our netty modules we are relocating here. We talked offline and ideally I wish we had an arrow distribution which shaded netty so we wouldn't have to play this game of excluding netty everywhere then adding back in a specific runtime version just to get the shading correctly so that Arrow will have the correct version of netty.
That said for now I think just being explicit will help us in the future to remember what we are doing here 🤞
There was a problem hiding this comment.
The only problem is that we relocate packages. That's why I'll have to relocate io.netty.util even though it comes from netty-common. If netty-common adds classes under another package, we will miss to relocate them.
I do agree with being more specific in relocation, though.
There was a problem hiding this comment.
That's fair. Again I wish we had an arrow distro that just did this for us, but such is life.
There was a problem hiding this comment.
Ugh, it is really unfortunate.
If I relocate everything under io.netty, it won't be safe if we start pulling netty libs from somewhere. If I relocate io.netty.util, nobody can guarantee a future version of netty-common won't have classes in another package.
Let's probably keep the wildcard relocate as at least we are in control of our deps.
3 participants
This PR contains a change to relocate all Netty dependencies, including
netty-common. Prior to this, we relocated onlynetty-bufferandnetty-commonlibs were packaged in the runtime jar.Below is the content of the runtime jar before this change.