Skip to content

Conversation

@pan3793
Copy link
Member

@pan3793 pan3793 commented Nov 7, 2022

Why are the changes needed?

This PR revises the shaded rule of spark engine module, especially to make sure that netty native libs is shaded properly.

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

(kyuubi) ➜  apache-kyuubi git:(spark-engine-shade) tree externals/kyuubi-spark-sql-engine/target/unpacked -L 5 | grep -v class
externals/kyuubi-spark-sql-engine/target/unpacked
├── META-INF
│   ├── LICENSE
│   ├── MANIFEST.MF
│   ├── NOTICE
│   ├── io.netty.versions.properties
│   ├── native
│   │   ├── liborg_apache_kyuubi_shade_netty_transport_native_epoll_aarch_64.so
│   │   └── liborg_apache_kyuubi_shade_netty_transport_native_epoll_x86_64.so
│   ├── services
│   │   ├── org.apache.kyuubi.shade.io.grpc.LoadBalancerProvider
│   │   ├── org.apache.kyuubi.shade.io.grpc.ManagedChannelProvider
│   │   ├── org.apache.kyuubi.shade.io.grpc.NameResolverProvider
│   │   ├── org.apache.kyuubi.shade.io.grpc.ServerProvider
│   │   ├── org.apache.kyuubi.shade.io.vertx.core.spi.launcher.CommandFactory
│   │   ├── org.apache.spark.status.AppHistoryServerPlugin
│   │   └── reactor.blockhound.integration.BlockHoundIntegration
│   ├── versions
│   │   └── 11
│   │       └── io
│   │           └── vertx
│   └── vertx
│       └── vertx-version.txt
├── kyuubi-version-info.properties
├── log4j2-defaults.xml
├── org
│   └── apache
│       ├── kyuubi
│       │   ├── cli
│       │   ├── config
│       │   │   └── internal
│       │   ├── engine
│       │   │   └── spark
│       │   ├── events
│       │   │   └── handler
│       │   ├── ha
│       │   │   └── client
│       │   ├── operation
│       │   │   ├── log
│       │   │   └── meta
│       │   ├── reflection
│       │   ├── service
│       │   │   └── authentication
│       │   ├── session
│       │   ├── shade
│       │   │   ├── android
│       │   │   ├── com
│       │   │   ├── io
│       │   │   ├── net
│       │   │   └── org
│       │   └── util
│       └── spark
│           ├── api
│           │   └── python
│           ├── kyuubi
│           └── ui
└── python
    ├── execute_python.py
    └── kyuubi_util.py

40 directories, 211 files
  • Run test locally before make a pull request

@pan3793 pan3793 marked this pull request as ready for review November 7, 2022 13:17
@pan3793 pan3793 changed the title [BUILD] Revisit Kyuubi Spark engine shaded [BUILD] Revise Kyuubi Spark engine shaded Nov 7, 2022
@pan3793 pan3793 requested a review from hddong November 7, 2022 13:21
@pan3793 pan3793 marked this pull request as draft November 7, 2022 16:25
@pan3793
Copy link
Member Author

pan3793 commented Nov 7, 2022

There are incompatible changes in Netty for early Spark release, then the shade of Netty is required.

22/11/07 14:01:10 ERROR ManagedChannelImpl: [Channel<1>: (ip:///localhost:49165)] Uncaught exception in the SynchronizationContext. Panic!
java.lang.NoSuchMethodError: io.netty.buffer.PooledByteBufAllocator.<init>(ZIIIIIIZ)V
	at org.apache.kyuubi.shade.io.grpc.netty.Utils.createByteBufAllocator(Utils.java:180)
	at org.apache.kyuubi.shade.io.grpc.netty.Utils.access$000(Utils.java:75)
	at org.apache.kyuubi.shade.io.grpc.netty.Utils$ByteBufAllocatorPreferDirectHolder.<clinit>(Utils.java:98)
	at org.apache.kyuubi.shade.io.grpc.netty.Utils.getByteBufAllocator(Utils.java:148)
	at org.apache.kyuubi.shade.io.grpc.netty.NettyClientTransport.start(NettyClientTransport.java:233)

@github-actions github-actions bot added kind:infra license, community building, project builds, asf infra related, etc. module:ha labels Nov 7, 2022
@codecov-commenter
Copy link

codecov-commenter commented Nov 10, 2022

Codecov Report

Merging #3776 (03302c0) into master (02fe757) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 03302c0 differs from pull request most recent head 6a6e2a6. Consider uploading reports for the commit 6a6e2a6 to get more accurate results

@@             Coverage Diff              @@
##             master    #3776      +/-   ##
============================================
- Coverage     52.87%   52.85%   -0.02%     
  Complexity       13       13              
============================================
  Files           496      496              
  Lines         27971    27967       -4     
  Branches       3857     3856       -1     
============================================
- Hits          14789    14782       -7     
  Misses        11788    11788              
- Partials       1394     1397       +3     
Impacted Files Coverage Δ
...rg/apache/kyuubi/ctl/cmd/log/LogBatchCommand.scala 63.93% <0.00%> (-1.64%) ⬇️
...yuubi/server/metadata/jdbc/JDBCMetadataStore.scala 89.27% <0.00%> (-0.70%) ⬇️
...a/org/apache/kyuubi/service/TFrontendService.scala 90.98% <0.00%> (-0.30%) ⬇️
...main/scala/org/apache/kyuubi/ctl/util/Render.scala 96.20% <0.00%> (+1.02%) ⬆️

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

@pan3793 pan3793 marked this pull request as ready for review November 10, 2022 09:29
@pan3793
Copy link
Member Author

pan3793 commented Nov 10, 2022

ping @hddong, it's ready for reviewing and testing

Copyright 2006-2010 The Apache Software Foundation.

Google Guava Version 18.0
* Copyright (C) 2009 The Guava Authors
Copy link
Contributor

Choose a reason for hiding this comment

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

Accidentally deleted? Seems Guava still be include.

Copy link
Member Author

Choose a reason for hiding this comment

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

The recent version Guava does not ship NOTICE in the jar

Copy link
Contributor

Choose a reason for hiding this comment

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

Get, LGTM, I will have a test locally.

@hddong
Copy link
Contributor

hddong commented Nov 11, 2022

Checked locally, it works as expected.

@pan3793 pan3793 self-assigned this Nov 11, 2022
@pan3793 pan3793 added this to the v1.7.0 milestone Nov 11, 2022
@pan3793
Copy link
Member Author

pan3793 commented Nov 11, 2022

Thanks, merging to master

@pan3793 pan3793 closed this in 72c1f53 Nov 11, 2022
@pan3793 pan3793 deleted the spark-engine-shade branch November 11, 2022 03:28
pan3793 added a commit that referenced this pull request Dec 2, 2022
This PR revises the shaded rule of spark engine module, especially to make sure that netty native libs is shaded properly.

- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible

- [x] Add screenshots for manual tests if appropriate

```
(kyuubi) ➜  apache-kyuubi git:(spark-engine-shade) tree externals/kyuubi-spark-sql-engine/target/unpacked -L 5 | grep -v class
externals/kyuubi-spark-sql-engine/target/unpacked
├── META-INF
│   ├── LICENSE
│   ├── MANIFEST.MF
│   ├── NOTICE
│   ├── io.netty.versions.properties
│   ├── native
│   │   ├── liborg_apache_kyuubi_shade_netty_transport_native_epoll_aarch_64.so
│   │   └── liborg_apache_kyuubi_shade_netty_transport_native_epoll_x86_64.so
│   ├── services
│   │   ├── org.apache.kyuubi.shade.io.grpc.LoadBalancerProvider
│   │   ├── org.apache.kyuubi.shade.io.grpc.ManagedChannelProvider
│   │   ├── org.apache.kyuubi.shade.io.grpc.NameResolverProvider
│   │   ├── org.apache.kyuubi.shade.io.grpc.ServerProvider
│   │   ├── org.apache.kyuubi.shade.io.vertx.core.spi.launcher.CommandFactory
│   │   ├── org.apache.spark.status.AppHistoryServerPlugin
│   │   └── reactor.blockhound.integration.BlockHoundIntegration
│   ├── versions
│   │   └── 11
│   │       └── io
│   │           └── vertx
│   └── vertx
│       └── vertx-version.txt
├── kyuubi-version-info.properties
├── log4j2-defaults.xml
├── org
│   └── apache
│       ├── kyuubi
│       │   ├── cli
│       │   ├── config
│       │   │   └── internal
│       │   ├── engine
│       │   │   └── spark
│       │   ├── events
│       │   │   └── handler
│       │   ├── ha
│       │   │   └── client
│       │   ├── operation
│       │   │   ├── log
│       │   │   └── meta
│       │   ├── reflection
│       │   ├── service
│       │   │   └── authentication
│       │   ├── session
│       │   ├── shade
│       │   │   ├── android
│       │   │   ├── com
│       │   │   ├── io
│       │   │   ├── net
│       │   │   └── org
│       │   └── util
│       └── spark
│           ├── api
│           │   └── python
│           ├── kyuubi
│           └── ui
└── python
    ├── execute_python.py
    └── kyuubi_util.py

40 directories, 211 files
```

- [ ] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request

Closes #3776 from pan3793/spark-engine-shade.

Closes #3776

6a6e2a6 [Cheng Pan] nit
e247923 [Cheng Pan] 1
a53b7c0 [Cheng Pan] nit
19382ef [Cheng Pan] [BUILD] Revisit Kyuubi Spark engine shaded

Authored-by: Cheng Pan <chengpan@apache.org>
Signed-off-by: Cheng Pan <chengpan@apache.org>
@pan3793 pan3793 modified the milestones: v1.7.0, v1.6.2 Dec 2, 2022
@pan3793
Copy link
Member Author

pan3793 commented Dec 2, 2022

Cherry picked to branch-1.6 as user reported that jetcd class conflict w/ TiSpark

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

Labels

kind:build kind:infra license, community building, project builds, asf infra related, etc. module:ha module:spark

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants