You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kyuubi.apache.org by GitBox <gi...@apache.org> on 2022/02/22 02:01:57 UTC

[GitHub] [incubator-kyuubi] SteNicholas opened a new pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

SteNicholas opened a new pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953


   <!--
   Thanks for sending a pull request!
   
   Here are some tips for you:
     1. If this is your first time, please read our contributor guidelines: https://kyuubi.readthedocs.io/en/latest/community/contributions.html
     2. If the PR is related to an issue in https://github.com/apache/incubator-kyuubi/issues, add '[KYUUBI #XXXX]' in your PR title, e.g., '[KYUUBI #XXXX] Your PR title ...'.
     3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][KYUUBI #XXXX] Your PR title ...'.
   -->
   
   ### _Why are the changes needed?_
   <!--
   Please clarify why the changes are needed. For instance,
     1. If you add a feature, you can talk about the use case of it.
     2. If you fix a bug, you can clarify why it is a bug.
   -->
   Upgrade thrift version to 0.16.0.
   
   ### _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
   
   - [x] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request


-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on a change in pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#discussion_r811529688



##########
File path: pom.xml
##########
@@ -519,11 +520,6 @@
                 <version>${kubernetes-client.version}</version>
             </dependency>
 
-            <!--
-              because of THRIFT-4805, we don't upgrade to libthrift:0.12.0,
-              because of THRIFT-5274, we don't upgrade to libthrift:0.13.0,
-              so just keep hive-service-rpc:2.3.9 transitive dependency libthrift:0.9.3
-            -->

Review comment:
       Then we can exclude *:* from `hive-service-rpc`, and add fb303 and thrift explicitly




-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] pan3793 edited a comment on pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
pan3793 edited a comment on pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#issuecomment-1047365031


   We need to shade thrift libs into kyuubi-spark-sql-engine because Spark bundles old thrift libs in binary release.


-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] pan3793 edited a comment on pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
pan3793 edited a comment on pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#issuecomment-1048481112


   Summary of this PR,
   1. Upgrade `libthrift` to 0.16.0 due to CVE issues and the coming upstream change of Spark
   2. Shade and relocate `thrift` and `hive-service-rpc` classes in `kyuubi-spark-engine`, it's necessary to avoding conflict with old thrift libs bundled in Spark binary releases.
   3. Due to thrift change the method signature, the subclasses of Kyuubi also need to change to pass compile.
   4. We rely on Hive 2.3.9 jars in some components, e.g. `kyuubi-hive-jdbc`, `LocalMetaServer` in `kyuubi-server` test classes. Some classes in Hive jars compiled against old thrift classes which are not compatible with thrift 0.16.0, which cause runtime link error, we found all broken tests and copy the broken classes with necessary modification to make it work with thrift 0.16.0, but there may still risk somewhere the UT not covered.


-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] yaooqinn commented on pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
yaooqinn commented on pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#issuecomment-1048617271


   the test failure here seems relevant, can we check on that? @SteNicholas  


-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on a change in pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#discussion_r812584268



##########
File path: kyuubi-server/src/test/scala/org/apache/kyuubi/credentials/HiveDelegationTokenProviderSuite.scala
##########
@@ -93,7 +93,8 @@ class HiveDelegationTokenProviderSuite extends KerberizedTestHelper {
     FileUtils.deleteDirectory(hadoopConfDir)
   }
 
-  test("obtain hive delegation token") {
+  // Ignore the test because LocalMetaServer can not work with Thrift 0.16.0.
+  ignore("obtain hive delegation token") {

Review comment:
       cc @zhouyifan279 
   We can not start `HiveMetastore` using thrift 0.16.0, because the Interface method signature changed, the class is not compatible.




-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
pan3793 commented on pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#issuecomment-1047365031


   We need to shaded thrift libs into kyuubi-spark-sql-engine because Spark bundles old thrift libs in binary release.


-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] pan3793 edited a comment on pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
pan3793 edited a comment on pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#issuecomment-1048481112


   Summary of this PR,
   1. Upgrade `libthrift` to 0.16.0 due to CVE issues and the coming upstream change of Spark
   2. Shade and relocate `thrift` and `hive-service-rpc` classes in `kyuubi-spark-engine`, it's necessary to avoding conflict with old thrift libs bundled in Spark binary releases.
   3. Due to thrift change the method signature, the subclasses of Kyuubi also need to change to pass compile.
   4. We rely on Hive 2.3.9 jars in some components, e.g. `kyuubi-hive-jdbc`, `LocalMetaServer` in `kyuubi-server` test classes. Some classes in Hive jars compiled against old thrift interfaces which are not compatible with thrift 0.16.0, it causes runtime link error, we found all broken tests and copy the broken classes with necessary modification to make it work with thrift 0.16.0, but there may still risk somewhere the UT not covered.


-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#discussion_r812717401



##########
File path: externals/kyuubi-spark-sql-engine/pom.xml
##########
@@ -191,6 +191,8 @@
                             <include>org.apache.curator:curator-client</include>
                             <include>org.apache.curator:curator-framework</include>
                             <include>org.apache.curator:curator-recipes</include>
+                            <include>org.apache.thrift:*</include>

Review comment:
       shall we not use *？




-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] yaooqinn commented on pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
yaooqinn commented on pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#issuecomment-1048752506


   thanks, merged to master


-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] yaooqinn commented on pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
yaooqinn commented on pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#issuecomment-1048614631


   Thanks for your summary, @pan3793
   > Some classes in Hive jars compiled against old thrift interfaces which are not compatible with thrift 0.16.0, it causes runtime link error, we found all broken tests and copy the broken classes with necessary modification to make it work with thrift 0.16.0, but there may still risk somewhere the UT not covered.
   
   BTW, please change `Some classes ` exactly what they are, and move the summary to the PR desc.


-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] pan3793 edited a comment on pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
pan3793 edited a comment on pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#issuecomment-1048481112


   Summary of this PR,
   1. Upgrade `libthrift` to 0.16.0 due to CVE issues and the coming upstream change of Spark
   2. Shade and relocate `thrift` and `hive-service-rpc` classes in `kyuubi-spark-engine`, it's necessary to avoding conflict with old thrift libs bundled in Spark binary releases.
   3. Due to thrift change the method signature, the subclasses of Kyuubi also need to change to pass compile.
   4. We rely on Hive 2.3.9 jars in some components, e.g. `kyuubi-hive-jdbc`, `LocalMetaServer` in `kyuubi-server` test classes. Some classes in Hive jars compiled against old thrift interfaces which are not compatible with thrift 0.16.0, which cause runtime link error, we found all broken tests and copy the broken classes with necessary modification to make it work with thrift 0.16.0, but there may still risk somewhere the UT not covered.


-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] yanghua commented on a change in pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
yanghua commented on a change in pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#discussion_r812832533



##########
File path: kyuubi-server/src/test/scala/org/apache/kyuubi/credentials/HiveDelegationTokenProviderSuite.scala
##########
@@ -93,7 +93,8 @@ class HiveDelegationTokenProviderSuite extends KerberizedTestHelper {
     FileUtils.deleteDirectory(hadoopConfDir)
   }
 
-  test("obtain hive delegation token") {
+  // Ignore the test because LocalMetaServer can not work with Thrift 0.16.0.
+  ignore("obtain hive delegation token") {

Review comment:
       > Shall we govern these ignored tests with flaky tests together? cc @yanghua
   
   I suggest we split them into two categories. The ignored tests may have other reasons not only flaky tests?




-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
pan3793 commented on pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#issuecomment-1048481112


   Summary of this PR,
   1. Upgrade `libthrift` to 0.16.0 due to CVE issues and the coming upstream change of Spark
   2. Shade and relocate `thrift` and `hive-service-rpc` classes in `kyuubi-spark-engine`, it's necessary to avoding conflict with old thrift libs bundled in Spark binary releases.
   3. Due to thrift change the method signature, the subclasses of Kyuubi also need to change to make compile passed.
   4. We rely on Hive 2.3.9 jars in some components, e.g. `kyuubi-hive-jdbc`, `LocalMetaServer` in `kyuubi-server` test classes, some classes in Hive jars compiled against old thrift classes which are not compatible with thrift 0.16.0, which cause runtime link error, we find all broken tests and copy the broken classes with necessary modification to make it work with thrift 0.16.0, but there may still risk somewhere the UT not covered.


-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] zhouyifan279 commented on a change in pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
zhouyifan279 commented on a change in pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#discussion_r812759402



##########
File path: kyuubi-server/src/test/scala/org/apache/kyuubi/credentials/HiveDelegationTokenProviderSuite.scala
##########
@@ -93,7 +93,8 @@ class HiveDelegationTokenProviderSuite extends KerberizedTestHelper {
     FileUtils.deleteDirectory(hadoopConfDir)
   }
 
-  test("obtain hive delegation token") {
+  // Ignore the test because LocalMetaServer can not work with Thrift 0.16.0.
+  ignore("obtain hive delegation token") {

Review comment:
       I will create a PR later to fix it.




-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on a change in pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#discussion_r811555575



##########
File path: externals/kyuubi-spark-sql-engine/pom.xml
##########
@@ -201,6 +201,13 @@
                                 <include>org.apache.curator.**</include>
                             </includes>
                         </relocation>
+                        <relocation>
+                            <pattern>org.apache.thrift</pattern>
+                            <shadedPattern>${kyuubi.shade.packageName}.org.apache.thrift</shadedPattern>
+                            <includes>
+                                <include>org.apache.thrift.**</include>
+                            </includes>
+                        </relocation>

Review comment:
       Should also be declared at L193




-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on a change in pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#discussion_r811556019



##########
File path: dev/dependencyList
##########
@@ -69,8 +69,6 @@ jetty-servlet/9.4.41.v20210516//jetty-servlet-9.4.41.v20210516.jar
 jetty-util-ajax/9.4.41.v20210516//jetty-util-ajax-9.4.41.v20210516.jar
 jetty-util/9.4.41.v20210516//jetty-util-9.4.41.v20210516.jar
 jline/0.9.94//jline-0.9.94.jar
-libfb303/0.9.3//libfb303-0.9.3.jar
-libthrift/0.9.3//libthrift-0.9.3.jar

Review comment:
       Why them disappeared?




-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on a change in pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#discussion_r811530483



##########
File path: pom.xml
##########
@@ -1307,6 +1303,22 @@
             <artifactId>scalatest_${scala.binary.version}</artifactId>
             <scope>test</scope>
         </dependency>
+
+        <dependency>

Review comment:
       move to L555




-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] pan3793 edited a comment on pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
pan3793 edited a comment on pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#issuecomment-1048481112


   Summary of this PR,
   1. Upgrade `libthrift` to 0.16.0 due to CVE issues and the coming upstream change of Spark
   2. Shade and relocate `thrift` and `hive-service-rpc` classes in `kyuubi-spark-engine`, it's necessary to avoding conflict with old thrift libs bundled in Spark binary releases.
   3. Due to thrift change the method signature, the subclasses of Kyuubi also need to change to make compile passed.
   4. We rely on Hive 2.3.9 jars in some components, e.g. `kyuubi-hive-jdbc`, `LocalMetaServer` in `kyuubi-server` test classes, some classes in Hive jars compiled against old thrift classes which are not compatible with thrift 0.16.0, which cause runtime link error, we found all broken tests and copy the broken classes with necessary modification to make it work with thrift 0.16.0, but there may still risk somewhere the UT not covered.


-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] yaooqinn closed pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
yaooqinn closed pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953


   


-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on a change in pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#discussion_r811529404



##########
File path: pom.xml
##########
@@ -133,6 +133,7 @@
         <scopt.version>4.0.1</scopt.version>
         <slf4j.version>1.7.35</slf4j.version>
         <log4j.version>2.17.1</log4j.version>
+        <libthrift.version>0.16.0</libthrift.version>

Review comment:
       Let's define `fb303.version` and `thrift.version`




-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] codecov-commenter commented on pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#issuecomment-1048570024


   # [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1953?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#1953](https://codecov.io/gh/apache/incubator-kyuubi/pull/1953?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (898effc) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/f25e5c93b8848524927f8e0c08e3d533a745a580?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f25e5c9) will **decrease** coverage by `0.53%`.
   > The diff coverage is `1.49%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-kyuubi/pull/1953/graphs/tree.svg?width=650&height=150&src=pr&token=925D4tb9AH&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/incubator-kyuubi/pull/1953?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #1953      +/-   ##
   ============================================
   - Coverage     60.89%   60.35%   -0.54%     
     Complexity       69       69              
   ============================================
     Files           302      305       +3     
     Lines         14797    14859      +62     
     Branches       1915     1927      +12     
   ============================================
   - Hits           9010     8968      -42     
   - Misses         5020     5111      +91     
   - Partials        767      780      +13     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/1953?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...in/scala/org/apache/kyuubi/config/KyuubiConf.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1953/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jb25maWcvS3l1dWJpQ29uZi5zY2FsYQ==) | `96.22% <ø> (-0.09%)` | :arrow_down: |
   | [...apache/kyuubi/service/TBinaryFrontendService.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1953/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2aWNlL1RCaW5hcnlGcm9udGVuZFNlcnZpY2Uuc2NhbGE=) | `88.09% <ø> (-1.27%)` | :arrow_down: |
   | [...a/org/apache/kyuubi/service/TFrontendService.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1953/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2aWNlL1RGcm9udGVuZFNlcnZpY2Uuc2NhbGE=) | `94.79% <0.00%> (-0.72%)` | :arrow_down: |
   | [.../authentication/HadoopThriftAuthBridgeServer.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1953/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2aWNlL2F1dGhlbnRpY2F0aW9uL0hhZG9vcFRocmlmdEF1dGhCcmlkZ2VTZXJ2ZXIuc2NhbGE=) | `60.82% <ø> (ø)` | |
   | [...ervice/authentication/TSetIpAddressProcessor.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1953/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2aWNlL2F1dGhlbnRpY2F0aW9uL1RTZXRJcEFkZHJlc3NQcm9jZXNzb3Iuc2NhbGE=) | `77.41% <ø> (ø)` | |
   | [...rg/apache/hadoop/hive/thrift/TFilterTransport.java](https://codecov.io/gh/apache/incubator-kyuubi/pull/1953/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWhpdmUtamRiYy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaGFkb29wL2hpdmUvdGhyaWZ0L1RGaWx0ZXJUcmFuc3BvcnQuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [...he/hadoop/hive/thrift/TUGIContainingTransport.java](https://codecov.io/gh/apache/incubator-kyuubi/pull/1953/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWhpdmUtamRiYy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaGFkb29wL2hpdmUvdGhyaWZ0L1RVR0lDb250YWluaW5nVHJhbnNwb3J0LmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...doop/hive/thrift/client/TUGIAssumingTransport.java](https://codecov.io/gh/apache/incubator-kyuubi/pull/1953/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWhpdmUtamRiYy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvaGFkb29wL2hpdmUvdGhyaWZ0L2NsaWVudC9UVUdJQXNzdW1pbmdUcmFuc3BvcnQuamF2YQ==) | `0.00% <0.00%> (ø)` | |
   | [.../org/apache/kyuubi/jdbc/hive/KyuubiConnection.java](https://codecov.io/gh/apache/incubator-kyuubi/pull/1953/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWhpdmUtamRiYy9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUva3l1dWJpL2pkYmMvaGl2ZS9LeXV1YmlDb25uZWN0aW9uLmphdmE=) | `4.37% <0.00%> (ø)` | |
   | [.../apache/kyuubi/client/KyuubiSyncThriftClient.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/1953/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jbGllbnQvS3l1dWJpU3luY1RocmlmdENsaWVudC5zY2FsYQ==) | `91.66% <100.00%> (ø)` | |
   | ... and [14 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/1953/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1953?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/1953?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [f25e5c9...898effc](https://codecov.io/gh/apache/incubator-kyuubi/pull/1953?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on a change in pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#discussion_r812585360



##########
File path: kyuubi-hive-jdbc/src/main/java/org/apache/hadoop/hive/thrift/TFilterTransport.java
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hive.thrift;
+
+import org.apache.thrift.TConfiguration;
+import org.apache.thrift.transport.TTransport;
+import org.apache.thrift.transport.TTransportException;
+
+/**
+ * Transport that simply wraps another transport. This is the equivalent of FilterInputStream for
+ * Thrift transports.
+ */
+public class TFilterTransport extends TTransport {
+
+  protected final TTransport wrapped;
+
+  public TFilterTransport(TTransport wrapped) {
+    this.wrapped = wrapped;
+  }
+
+  @Override
+  public void open() throws TTransportException {
+    wrapped.open();
+  }
+
+  @Override
+  public boolean isOpen() {
+    return wrapped.isOpen();
+  }
+
+  @Override
+  public boolean peek() {
+    return wrapped.peek();
+  }
+
+  @Override
+  public void close() {
+    wrapped.close();
+  }
+
+  @Override
+  public int read(byte[] buf, int off, int len) throws TTransportException {
+    return wrapped.read(buf, off, len);
+  }
+
+  @Override
+  public int readAll(byte[] buf, int off, int len) throws TTransportException {
+    return wrapped.readAll(buf, off, len);
+  }
+
+  @Override
+  public void write(byte[] buf) throws TTransportException {
+    wrapped.write(buf);
+  }
+
+  @Override
+  public void write(byte[] buf, int off, int len) throws TTransportException {
+    wrapped.write(buf, off, len);
+  }
+
+  @Override
+  public void flush() throws TTransportException {
+    wrapped.flush();
+  }
+
+  @Override
+  public byte[] getBuffer() {
+    return wrapped.getBuffer();
+  }
+
+  @Override
+  public int getBufferPosition() {
+    return wrapped.getBufferPosition();
+  }
+
+  @Override
+  public int getBytesRemainingInBuffer() {
+    return wrapped.getBytesRemainingInBuffer();
+  }
+
+  @Override
+  public void consumeBuffer(int len) {
+    wrapped.consumeBuffer(len);
+  }
+
+  @Override
+  public TConfiguration getConfiguration() {
+    return null;
+  }
+
+  @Override
+  public void updateKnownMessageSize(long size) throws TTransportException {}

Review comment:
       cc @yaooqinn 
   We need to copy this and some related classes from Hive Jars and rebuild with thrift 0.16.0. Because `TTransport` changed.




-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#discussion_r812721700



##########
File path: kyuubi-server/src/test/scala/org/apache/kyuubi/credentials/HiveDelegationTokenProviderSuite.scala
##########
@@ -93,7 +93,8 @@ class HiveDelegationTokenProviderSuite extends KerberizedTestHelper {
     FileUtils.deleteDirectory(hadoopConfDir)
   }
 
-  test("obtain hive delegation token") {
+  // Ignore the test because LocalMetaServer can not work with Thrift 0.16.0.
+  ignore("obtain hive delegation token") {

Review comment:
       Shall we govern these ignored tests with flaky tests together? cc @yanghua 




-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on a change in pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#discussion_r811529688



##########
File path: pom.xml
##########
@@ -519,11 +520,6 @@
                 <version>${kubernetes-client.version}</version>
             </dependency>
 
-            <!--
-              because of THRIFT-4805, we don't upgrade to libthrift:0.12.0,
-              because of THRIFT-5274, we don't upgrade to libthrift:0.13.0,
-              so just keep hive-service-rpc:2.3.9 transitive dependency libthrift:0.9.3
-            -->

Review comment:
       Then we can exclude `*:*` from `hive-service-rpc`, and add fb303 and thrift explicitly




-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on a change in pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#discussion_r811562449



##########
File path: dev/dependencyList
##########
@@ -20,6 +20,7 @@ classgraph/4.8.95//classgraph-4.8.95.jar
 commons-codec/1.15//commons-codec-1.15.jar
 commons-lang/2.6//commons-lang-2.6.jar
 commons-lang3/3.10//commons-lang3-3.10.jar
+commons-logging/1.2//commons-logging-1.2.jar

Review comment:
       `commons-logging` should be excluded.




-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on a change in pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
pan3793 commented on a change in pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#discussion_r811562666



##########
File path: dev/dependencyList
##########
@@ -36,6 +37,8 @@ hk2-api/2.6.1//hk2-api-2.6.1.jar
 hk2-locator/2.6.1//hk2-locator-2.6.1.jar
 hk2-utils/2.6.1//hk2-utils-2.6.1.jar
 htrace-core4/4.1.0-incubating//htrace-core4-4.1.0-incubating.jar
+httpclient/4.5.13//httpclient-4.5.13.jar
+httpcore/4.4.15//httpcore-4.4.15.jar

Review comment:
       Add them LGTM, prepare for Thrift-HTTP mode.




-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] pan3793 edited a comment on pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
pan3793 edited a comment on pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#issuecomment-1048481112


   Summary of this PR,
   1. Upgrade `libthrift` to 0.16.0 due to CVE issues and the coming upstream change of Spark
   2. Shade and relocate `thrift` and `hive-service-rpc` classes in `kyuubi-spark-engine`, it's necessary to avoding conflict with old thrift libs bundled in Spark binary releases.
   3. Due to thrift change the method signature, the subclasses of Kyuubi also need to change to pass compile.
   4. We rely on Hive 2.3.9 jars in some components, e.g. `kyuubi-hive-jdbc`, `LocalMetaServer` in `kyuubi-server` test classes, some classes in Hive jars compiled against old thrift classes which are not compatible with thrift 0.16.0, which cause runtime link error, we found all broken tests and copy the broken classes with necessary modification to make it work with thrift 0.16.0, but there may still risk somewhere the UT not covered.


-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] pan3793 commented on pull request #1953: [KYUUBI #1948] Upgrade thrift version to 0.16.0

Posted by GitBox <gi...@apache.org>.
pan3793 commented on pull request #1953:
URL: https://github.com/apache/incubator-kyuubi/pull/1953#issuecomment-1048646933


   Updated PR description, also added **Next Steps** @yaooqinn 


-- 
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: commits-unsubscribe@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org