-
Notifications
You must be signed in to change notification settings - Fork 919
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
[KYUUBI #6411] Grpc common support #6412
Conversation
1. "$@" is a array, we want use string to compare. so update "$@" => "$*" 2. `tty` mean execute the command, we can use $(tty) replace it 3. param $# is a number, compare number should use -gt/-lt,not >/<
1. "$@" is a array, we want use string to compare. so update "$@" => "$*" 2. `tty` mean execute the command, we can use $(tty) replace it 3. param $# is a number, compare number should use -gt/-lt,not >/< 4. not sure the /bin/kyuubi line 63 'exit -1' need modify? so the directory bin only have a shellcheck note in /bin/kyuubi
# Conflicts: # bin/docker-image-tool.sh
Revert "fix_4186"
# Conflicts: # externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/SQLOperationListener.scala # externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/kyuubi/SparkConsoleProgressBar.scala
…ommon-support # Conflicts: # kyuubi-grpc/pom.xml
@wForget @yaooqinn @ulysses-you This pr is to add common-grpc module based on #6365, i add a simple test case with two simple grpc request/response, what do you think this. |
Thank you @davidyuan1223 , I think we should make the first commit goal clear. Custom grpc things do not help us support Spark connect (the key make ExecutePlanRequest to be compatible with Kyuubi ?). I think we can add a test: use Spark connect client to query/cancel on a Kyuubi grpc server. |
You are right, I will add a test case to test spark connect client with kyuubi server |
i'm trying to build a simple case to test spark grpc, but it always report Error
I have tried to solve this issue many times by searching for a solution on https://issues.apache.org/jira/browse/SPARK-45201, but it seems futile. Can we focus on discussing the structure instead? We can then use the new module kyuubi-grpc-spark to test this. This bug is quite challenging for me. |
@davidyuan1223 thank you for the work. Do you require this patch apache/spark#45775 ? This issue is due to connect required a higher guava version which is different with Spark core itself, but connect depends on the Spark core's shaded guava. I think this issue has been fixed in Spark3.5.2 which is on RCing, we can use the 3.5.2 rc version to verify it and change to use 3.5.2 after released. |
thanks, i will try! The spark-connect package has troubled me for a long time. Its proto classes conflict with connect-common, forcing me to shade it. Then I encountered a problem with Guava, and despite trying many methods, it still didn't work. |
thank you @davidyuan1223 , if you find any other issues related to Spark, just craete PR to Spark community. We can help review and push it. |
Thanks for the PR! This PR is being closed due to inactivity. This isn't a judgement on the merit of the PR in any way. If this is still an issue with the latest version of Kyuubi, please reopen it and ask a committer to remove the Stale tag! Thank you for using Kyuubi! |
🔍 Description
Issue References 🔗
This pull request fixes #6411
Describe Your Solution 🔧
Add a common module common-grpc to support grpc type fronendservice/backendservice, which can compitable with another grpc engine, not only spark connect
Types of changes 🔖
Test Plan 🧪
Behavior Without This Pull Request ⚰️
Behavior With This Pull Request 🎉
Related Unit Tests
Checklist 📝
Be nice. Be informative.