You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/11/02 19:42:19 UTC
[GitHub] [iceberg] aokolnychyi opened a new pull request, #6107: Spark 3.3: Relocate all Netty dependencies
aokolnychyi opened a new pull request, #6107:
URL: https://github.com/apache/iceberg/pull/6107
This PR contains a change to relocate all Netty dependencies, including `netty-common`. Prior to this, we relocated only `netty-buffer` and `netty-common` libs were packaged in the runtime jar.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #6107: Spark 3.3: Relocate all Netty dependencies
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on code in PR #6107:
URL: https://github.com/apache/iceberg/pull/6107#discussion_r1012234582
##########
spark/v3.3/build.gradle:
##########
@@ -65,8 +65,9 @@ project(":iceberg-spark:iceberg-spark-${sparkMajorVersion}_${scalaVersion}") {
exclude group: 'org.apache.avro', module: 'avro'
exclude group: 'org.apache.arrow'
exclude group: 'org.apache.parquet'
- // 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'
Review Comment:
We exclude `netty-common` from many deps but not all. I feel it is safer to be consistent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #6107: Spark 3.3: Relocate all Netty dependencies
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on code in PR #6107:
URL: https://github.com/apache/iceberg/pull/6107#discussion_r1012282476
##########
spark/v3.3/build.gradle:
##########
@@ -258,7 +261,7 @@ project(":iceberg-spark:iceberg-spark-runtime-${sparkMajorVersion}_${scalaVersio
relocate 'io.airlift', 'org.apache.iceberg.shaded.io.airlift'
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'
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] flyrain commented on a diff in pull request #6107: Spark 3.3: Relocate all Netty dependencies
Posted by GitBox <gi...@apache.org>.
flyrain commented on code in PR #6107:
URL: https://github.com/apache/iceberg/pull/6107#discussion_r1012253861
##########
spark/v3.3/build.gradle:
##########
@@ -65,8 +65,9 @@ project(":iceberg-spark:iceberg-spark-${sparkMajorVersion}_${scalaVersion}") {
exclude group: 'org.apache.avro', module: 'avro'
exclude group: 'org.apache.arrow'
exclude group: 'org.apache.parquet'
- // 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'
Review Comment:
How about enforce the version instead of excluding here and there?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #6107: Spark 3.3: Relocate all Netty dependencies
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on code in PR #6107:
URL: https://github.com/apache/iceberg/pull/6107#discussion_r1012276093
##########
spark/v3.3/build.gradle:
##########
@@ -258,7 +261,7 @@ project(":iceberg-spark:iceberg-spark-runtime-${sparkMajorVersion}_${scalaVersio
relocate 'io.airlift', 'org.apache.iceberg.shaded.io.airlift'
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'
Review 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 🤞
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #6107: Spark 3.3: Relocate all Netty dependencies
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on code in PR #6107:
URL: https://github.com/apache/iceberg/pull/6107#discussion_r1012276093
##########
spark/v3.3/build.gradle:
##########
@@ -258,7 +261,7 @@ project(":iceberg-spark:iceberg-spark-runtime-${sparkMajorVersion}_${scalaVersio
relocate 'io.airlift', 'org.apache.iceberg.shaded.io.airlift'
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'
Review Comment:
I think we should be specific about our netty modules we are relocating here. We talked offline and ideally I wish we had a 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.
That said for now I think just being explicit will help us in the future to remember what we are doing here 🤞
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #6107: Spark 3.3: Relocate all Netty dependencies
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on code in PR #6107:
URL: https://github.com/apache/iceberg/pull/6107#discussion_r1012287617
##########
spark/v3.3/build.gradle:
##########
@@ -258,7 +261,7 @@ project(":iceberg-spark:iceberg-spark-runtime-${sparkMajorVersion}_${scalaVersio
relocate 'io.airlift', 'org.apache.iceberg.shaded.io.airlift'
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'
Review 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 nitty-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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #6107: Spark 3.3: Relocate all Netty dependencies
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on code in PR #6107:
URL: https://github.com/apache/iceberg/pull/6107#discussion_r1012284675
##########
spark/v3.3/build.gradle:
##########
@@ -65,8 +65,9 @@ project(":iceberg-spark:iceberg-spark-${sparkMajorVersion}_${scalaVersion}") {
exclude group: 'org.apache.avro', module: 'avro'
exclude group: 'org.apache.arrow'
exclude group: 'org.apache.parquet'
- // 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'
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi merged pull request #6107: Spark 3.3: Relocate all Netty dependencies
Posted by GitBox <gi...@apache.org>.
aokolnychyi merged PR #6107:
URL: https://github.com/apache/iceberg/pull/6107
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a diff in pull request #6107: Spark 3.3: Relocate all Netty dependencies
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on code in PR #6107:
URL: https://github.com/apache/iceberg/pull/6107#discussion_r1012287617
##########
spark/v3.3/build.gradle:
##########
@@ -258,7 +261,7 @@ project(":iceberg-spark:iceberg-spark-runtime-${sparkMajorVersion}_${scalaVersio
relocate 'io.airlift', 'org.apache.iceberg.shaded.io.airlift'
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'
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] flyrain commented on a diff in pull request #6107: Spark 3.3: Relocate all Netty dependencies
Posted by GitBox <gi...@apache.org>.
flyrain commented on code in PR #6107:
URL: https://github.com/apache/iceberg/pull/6107#discussion_r1012295406
##########
spark/v3.3/build.gradle:
##########
@@ -65,8 +65,9 @@ project(":iceberg-spark:iceberg-spark-${sparkMajorVersion}_${scalaVersion}") {
exclude group: 'org.apache.avro', module: 'avro'
exclude group: 'org.apache.arrow'
exclude group: 'org.apache.parquet'
- // 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'
Review Comment:
I don't see any problem of enforcing in this context. Either works for me, though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #6107: Spark 3.3: Relocate all Netty dependencies
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on code in PR #6107:
URL: https://github.com/apache/iceberg/pull/6107#discussion_r1012284131
##########
spark/v3.3/build.gradle:
##########
@@ -258,7 +261,7 @@ project(":iceberg-spark:iceberg-spark-runtime-${sparkMajorVersion}_${scalaVersio
relocate 'io.airlift', 'org.apache.iceberg.shaded.io.airlift'
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'
Review Comment:
That's fair. Again I wish we had an arrow distro that just did this for us, but such is life.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #6107: Spark 3.3: Relocate all Netty dependencies
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on PR #6107:
URL: https://github.com/apache/iceberg/pull/6107#issuecomment-1301154891
cc @flyrain @szehon-ho @RussellSpitzer @nastra @Fokko
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org