-
Notifications
You must be signed in to change notification settings - Fork 903
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
Switch to kyuubi-relocated-hive-service-rpc
#5783
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #5783 +/- ##
============================================
+ Coverage 61.27% 61.46% +0.19%
Complexity 23 23
============================================
Files 608 608
Lines 36050 35941 -109
Branches 4951 4940 -11
============================================
+ Hits 22088 22092 +4
+ Misses 11567 11457 -110
+ Partials 2395 2392 -3 ☔ View full report in Codecov by Sentry. |
kyuubi-shaded-hive-service-rpc
kyuubi-relocated-hive-service-rpc
…m to remove deps of snappy in ZK client 3.6 ### _Why are the changes needed?_ This is step 2 of #28 > 2. remove the Snappy support (simply throw `UnsupportedOperationException`) in `org.apache.zookeeper.server.persistence.SnapStream` and snappy deps ### _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 - [ ] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request - [x] Verified through apache/kyuubi#5783 Closes #29 from pan3793/snappy-2. a109d1c [Cheng Pan] [KYUUBI-SHADED #28] Step 1/2: Overwrite SnapStream to remove deps of snappy in ZK client 3.6 Authored-by: Cheng Pan <chengpan@apache.org> Signed-off-by: Cheng Pan <chengpan@apache.org>
…snappy in ZK client 3.6 ### _Why are the changes needed?_ This is step 2 of #28 > 2. remove the Snappy support (simply throw `UnsupportedOperationException`) in `org.apache.zookeeper.server.persistence.SnapStream` and snappy deps ### _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 - [ ] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request - [x] Verified through apache/kyuubi#5783 Closes #29 from pan3793/snappy-2. a109d1c [Cheng Pan] [KYUUBI-SHADED #28] Step 1/2: Overwrite SnapStream to remove deps of snappy in ZK client 3.6 Authored-by: Cheng Pan <chengpan@apache.org> Signed-off-by: Cheng Pan <chengpan@apache.org>
kyuubi-relocated-hive-service-rpc
kyuubi-relocated-hive-service-rpc
also cc @zhouyifan279 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks.
The title description needs to be updated, this pr also switches kyuubi-relocated-zookeeper-34
.
@wForget thanks for checking, I updated the PR description |
@@ -187,6 +187,21 @@ | |||
</exclusions> | |||
</dependency> | |||
|
|||
<dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to add a comment here to explain why these dependencies are retained?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry I missed this comment, it would be straightforward if the user searches the codebase with the keyword "import org.apache.thrift"
Thanks, merged to master |
🔍 Description
Issue References 🔗
TL;DR there are some issues with shading Thrift RPC classes during the engine packaging phase, see details in the PR description of apache/kyuubi-shaded#20.
Describe Your Solution 🔧
This PR aims to migrate from vanilla
hive-service-rpc
,libfb303
,libthrift
tokyuubi-relocated-hive-service-rpc
introduced in apache/kyuubi-shaded#20, the detailed works are:pom.xml
and rename the package prefix in all modules, except forkyuubi-server
there are a few places use vanilla thrift classes to access HMS to get tokenkyuubi-hive-sql-engine
Hive method invocationHiveRpcUtils
inkyuubi-hive-sql-engine
module for object conversion.As part of the whole change, this PR upgrades from the Kyuubi Shaded 0.1.0 to 0.2.0, which changes the jars name. see https://kyuubi.apache.org/shaded-release/0.2.0.html
Types of changes 🔖
Test Plan 🧪
Behavior Without This Pull Request ⚰️
Behavior With This Pull Request 🎉
Related Unit Tests
Pass all Hive UT with Hive 3.1.3, and IT with Hive 3.1.3 and 2.3.9 (also tested with 2.1.1-cdh6.3.2)
Checklists
📝 Author Self Checklist
📝 Committer Pre-Merge Checklist
Be nice. Be informative.