[SPARK-44263][CONNECT] Custom Interceptors Support #41880

cdkrot · 2023-07-06T14:31:03Z

What changes were proposed in this pull request?

Extend SparkSession to allow custom interceptors (grpc's ClientInterceptor)

Why are the changes needed?

This can be used for different purposes, such as customized authentication mechanisms

This is similar to Python which allows custom channel builders (#40993)

Does this PR introduce any user-facing change?

SparkSession allows custom interceptors. The proposed usage is

val interceptor = new ClientInterceptor {....}
val session = SparkSession.builder().interceptor(interceptor).create()

Or same with more than one interceptor

How was this patch tested?

UT

cdkrot · 2023-07-06T14:31:29Z

cc @grundprinzip, @nija-at

cdkrot · 2023-07-06T14:34:16Z

...nnect/client/jvm/src/main/scala/org/apache/spark/sql/connect/client/SparkConnectClient.scala

-
-    override def thisUsesUnstableApi(): Unit = {
-      // Marks this API is not stable. Left empty on purpose.
-    }


This API is no longer unstable (was stabilized) so this can be removed. See grpc/grpc-java#1914

grundprinzip · 2023-07-06T19:37:05Z

.../client/jvm/src/test/scala/org/apache/spark/sql/connect/client/SparkConnectClientSuite.scala

+      })
+      .build()
+
+    val session = SparkSession.builder().client(client).create()


The client() method is private[sql] for anyone providing any kinds of wrappers on top of the SparkSession for custom auth integrations it's not possible to use this method.

The proposed usage is rather

SparkSession.builder().interceptor(my interceptor).create(), and this one is public.

Perhaps I should add a test doing this exact line nearby

Now there is an external api test as well (in SparkSessionSuite)

grundprinzip

I think generally the approach goes in the right direction, we just need to figure out a better wrapping for the construction of the class.

grundprinzip

Looks good, lets polish and get it in.

...nnect/client/jvm/src/main/scala/org/apache/spark/sql/connect/client/SparkConnectClient.scala

grundprinzip · 2023-07-07T09:59:55Z

connector/connect/client/jvm/src/test/scala/org/apache/spark/sql/SparkSessionSuite.scala

+  private def startDummyServer(port: Int): Unit = {
+    service = new DummySparkConnectService
+    server = NettyServerBuilder
+      .forPort(port)
+      .addService(service)
+      .build()
+    server.start()
+  }


I think we have something called e2e client test for this that does properly start the service.

I guess you refer me to ClientE2ETestSuite? I will take a look. Here I was following how it's done in SparkConnectClientSuite. If there is a better way, we should replace how the server starts in both places

Check ClientE2ETestSuite which uses RemoteSparkSession which is probably what you want most.

Well I was going to complain that ClientE2ETestSuite is not a great fit to reuse, for some reasons such as it owns how the session is created and it lasts for the whole suite, but actually I realized that my test doesn't event need a server, so I dropped the server creation altogether

grundprinzip · 2023-07-07T10:32:02Z

connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/SparkSession.scala

@@ -656,6 +654,11 @@ object SparkSession extends Logging {
      this
    }

+    def interceptor(interceptor: ClientInterceptor): Builder = {


doc please :)

This is the public facing part of the interface and needs the most extensive documentation,.

Ok, good point :). I didn't do it because other public methods here are undocumented, but that's probably not a good idea either

...nnect/client/jvm/src/main/scala/org/apache/spark/sql/connect/client/SparkConnectClient.scala

grundprinzip · 2023-07-07T10:34:25Z

@cdkrot Please make the PR description very explicit, ideally with a code snippet of how to use it and why this is needed.

…ql/connect/client/SparkConnectClient.scala Co-authored-by: Martin Grund <[email protected]>

grundprinzip

Let's get the nits out the way and it's good to merge. @HyukjinKwon and @hvanhovell can you please have a look as well.

connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/SparkSession.scala

HyukjinKwon

I can merge when the tests pass.

cdkrot · 2023-07-10T13:46:17Z

I fixed tests, and also this weird SQL<->Connect comparison errors. Hope we can get this merged. cc @HyukjinKwon

HyukjinKwon · 2023-07-11T08:33:51Z

Merged to master.

### What changes were proposed in this pull request? Extend SparkSession to allow custom interceptors (grpc's ClientInterceptor) ### Why are the changes needed? This can be used for different purposes, such as customized authentication mechanisms This is similar to Python which allows custom channel builders (apache#40993) ### Does this PR introduce _any_ user-facing change? SparkSession allows custom interceptors. The proposed usage is ``` val interceptor = new ClientInterceptor {....} val session = SparkSession.builder().interceptor(interceptor).create() ``` Or same with more than one interceptor ### How was this patch tested? UT Closes apache#41880 from cdkrot/scala_channel_builder_2. Lead-authored-by: Alice Sayutina <[email protected]> Co-authored-by: Alice Sayutina <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>

cdkrot added 2 commits July 6, 2023 16:22

Support custom interceptors

38166e9

merge

724cceb

github-actions bot added SQL BUILD CONNECT labels Jul 6, 2023

cdkrot commented Jul 6, 2023

View reviewed changes

oops

ba78f84

github-actions bot removed the BUILD label Jul 6, 2023

grundprinzip reviewed Jul 6, 2023

View reviewed changes

cdkrot added 2 commits July 7, 2023 11:51

more tests

8a983c5

merge

1d4c4c6

cdkrot requested a review from grundprinzip July 7, 2023 09:53

cdkrot added 3 commits July 7, 2023 11:54

typo

5829b74

misc

7102646

misc

8e4997d

grundprinzip reviewed Jul 7, 2023

View reviewed changes

cdkrot and others added 2 commits July 7, 2023 12:43

Update connector/connect/client/jvm/src/main/scala/org/apache/spark/s…

d19aae8

…ql/connect/client/SparkConnectClient.scala Co-authored-by: Martin Grund <[email protected]>

Update connector/connect/client/jvm/src/main/scala/org/apache/spark/s…

4d48e2a

…ql/connect/client/SparkConnectClient.scala Co-authored-by: Martin Grund <[email protected]>

cdkrot mentioned this pull request Jul 7, 2023

[SPARK-44263][CONNECT] Channel Builder support #41807

Closed

cdkrot added 2 commits July 7, 2023 16:54

external documentation

887ea59

change SparkSessionSuite tests

2fc8138

cdkrot requested a review from grundprinzip July 7, 2023 15:51

grundprinzip approved these changes Jul 8, 2023

View reviewed changes

connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/SparkSession.scala Show resolved Hide resolved

connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/SparkSession.scala Show resolved Hide resolved

foreach

e609b8a

HyukjinKwon approved these changes Jul 10, 2023

View reviewed changes

cdkrot added 2 commits July 10, 2023 10:30

Merge branch 'master' into scala_channel_builder_2

475a217

style; imports

91d880e

cdkrot added 6 commits July 10, 2023 10:42

@SInCE

8bf8af4

style

5475873

don't have this api in sql

5c62a4b

don't have this api in sql (2)

ed93d22

ignore spark session builder in sql

92a511e

ignore spark session builder in sql

e730d50

HyukjinKwon closed this in eb07110 Jul 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-44263][CONNECT] Custom Interceptors Support #41880

[SPARK-44263][CONNECT] Custom Interceptors Support #41880

cdkrot commented Jul 6, 2023 •

edited

Loading

cdkrot commented Jul 6, 2023

cdkrot Jul 6, 2023 •

edited

Loading

grundprinzip Jul 6, 2023

cdkrot Jul 6, 2023 •

edited

Loading

cdkrot Jul 7, 2023

grundprinzip left a comment

grundprinzip left a comment

grundprinzip Jul 7, 2023

cdkrot Jul 7, 2023

grundprinzip Jul 7, 2023

cdkrot Jul 7, 2023

grundprinzip Jul 7, 2023

grundprinzip Jul 7, 2023

cdkrot Jul 7, 2023

grundprinzip commented Jul 7, 2023

grundprinzip left a comment

HyukjinKwon left a comment

cdkrot commented Jul 10, 2023

HyukjinKwon commented Jul 11, 2023

[SPARK-44263][CONNECT] Custom Interceptors Support #41880

[SPARK-44263][CONNECT] Custom Interceptors Support #41880

Conversation

cdkrot commented Jul 6, 2023 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

cdkrot commented Jul 6, 2023

cdkrot Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdkrot Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grundprinzip left a comment

Choose a reason for hiding this comment

grundprinzip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grundprinzip commented Jul 7, 2023

grundprinzip left a comment

Choose a reason for hiding this comment

HyukjinKwon left a comment

Choose a reason for hiding this comment

cdkrot commented Jul 10, 2023

HyukjinKwon commented Jul 11, 2023

cdkrot commented Jul 6, 2023 •

edited

Loading

cdkrot Jul 6, 2023 •

edited

Loading

cdkrot Jul 6, 2023 •

edited

Loading