Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

SparkConnectEngineDemo #6365

Closed

Conversation

davidyuan1223
Copy link
Contributor

@davidyuan1223 davidyuan1223 commented May 6, 2024

馃攳 Description

Issue References 馃敆

This pull request fixes #

Describe Your Solution 馃敡

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Types of changes 馃敄

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Test Plan 馃И

Behavior Without This Pull Request 鈿帮笍

Behavior With This Pull Request 馃帀

Related Unit Tests


Checklist 馃摑

Be nice. Be informative.

davidyuan1223 and others added 30 commits January 13, 2023 16:40
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:
#	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
currently only support config(not test)
currently only support config(not test)
@github-actions github-actions bot added kind:infra license, community building, project builds, asf infra related, etc. module:common kind:build labels May 6, 2024
@davidyuan1223 davidyuan1223 changed the title SparkConnectEngineDemo [Draft] SparkConnectEngineDemo May 6, 2024
@davidyuan1223 davidyuan1223 reopened this May 6, 2024
@davidyuan1223 davidyuan1223 marked this pull request as draft May 6, 2024 17:33
@davidyuan1223 davidyuan1223 changed the title [Draft] SparkConnectEngineDemo SparkConnectEngineDemo May 6, 2024
@davidyuan1223
Copy link
Contributor Author

@pan3793 @yaooqinn @ulysses-you this pr is only a draft for discussion. What do you think about this method to compatible spark grpc ?

@ulysses-you
Copy link
Contributor

Thank you @davidyuan1223 . We should add the grpc frontend service first, it is used to receive spark connect client request and send to connect engine.

@davidyuan1223
Copy link
Contributor Author

@ulysses-you what do you think about the new module kyuubi-grpc(only structure, not write test case yet)

Copy link
Contributor

Choose a reason for hiding this comment

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

@davidyuan1223 is it possbile add spark-connect dependency and just delegate request to the SparkConnectService ? If so, we do not need to copy proto files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ulysses-you yes, but i think, maybe we need to add other request/response, so my plan is put the proto files in kyuubi grpc, we can add kyuubi's interface based on the spark-connect proto, so not add the spark-connect common dependency

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate your plan ? I'm afrait things becomes complex if we decide to support connect based on proto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate your plan ? I'm afrait things becomes complex if we decide to support connect based on proto.

Actually, it's a assume, because the proto is based on spark, if we wanna support it with another engine, maybe need modify the proto? So i think put the proto in kyuubi-grpc could help us compite with other sql engine in the future. And if we wanna add spark-connect-common dependency, we need the spark version > 3.4, i think it's not compitable with other versions, because the rpc request will be handle by kyuubi's grpc backendService, not spark's rpc server, based on the proto, we could support rpc process in other spark version

Copy link
Contributor

Choose a reason for hiding this comment

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

So i think put the proto in kyuubi-grpc could help us compite with other sql engine in the future.

It's too early to consider that. Even if we add proto, there is no standard proto interface, how can Spark proto compatible with other engine ? For each proto based engine, I think we can also add their server dependency.

And if we wanna add spark-connect-common dependency, we need the spark version > 3.4, i think it's not compitable with other versions, because the rpc request will be handle by kyuubi's grpc backendService, not spark's rpc server, based on the proto, we could support rpc process in other spark version

It sounds like just like copy all Spark connect server code into Kyuubi repo ? I think we only need to support connect since Spark3.5 and later, that's why we need a new module for Kyuubi Server. We do not need support older Spark version.


Besides, Add a Kyuubi grpc proto is fine to me for those statements, e.g., show kyuubi session/operators etc which are for Kyuubi itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

consume

So i think put the proto in kyuubi-grpc could help us compite with other sql engine in the future.

It's too early to consider that. Even if we add proto, there is no standard proto interface, how can Spark proto compatible with other engine ? For each proto based engine, I think we can also add their server dependency.

And if we wanna add spark-connect-common dependency, we need the spark version > 3.4, i think it's not compitable with other versions, because the rpc request will be handle by kyuubi's grpc backendService, not spark's rpc server, based on the proto, we could support rpc process in other spark version

It sounds like just like copy all Spark connect server code into Kyuubi repo ? I think we only need to support connect since Spark3.5 and later, that's why we need a new module for Kyuubi Server. We do not need support older Spark version.

Besides, Add a Kyuubi grpc proto is fine to me for those statements, e.g., show kyuubi session/operators etc which are for Kyuubi itself.

You are right, i will modify them.

@ulysses-you
Copy link
Contributor

@ulysses-you what do you think about the new module kyuubi-grpc(only structure, not write test case yet)

@davidyuan1223 yeah, a new module for kyuubi grpc server is better. We can refactor and improve code after completing the connect frontend in future.

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:common
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants