[SPARK-40536][CONNECT] Make Spark Connect port configurable#38006
[SPARK-40536][CONNECT] Make Spark Connect port configurable#38006amaliujia wants to merge 5 commits intoapache:masterfrom
Conversation
amaliujia
changed the title
|
|
||
| package org.apache.spark.internal.config | ||
|
|
||
| private[spark] object Connect { |
There was a problem hiding this comment.
As Spark connect is build as plugin, all of the configuration should ideally be not located in core. Is there a way we can move this to the connect module?
There was a problem hiding this comment.
I agree. I am not sure if there is a way to have config only for a plugin. let's see other reviewer's suggestions on this.
There was a problem hiding this comment.
Yeah, it should better be placed within connect if possible. How do we use this plugin? I suspect the jar should be provided anyway (?)
There was a problem hiding this comment.
Does Spark Connect module depend on Spark Core? If yes, we can move this object to Spark Connect module.
There was a problem hiding this comment.
Yes it depends on core. Let me move this config and also address other comments.
There was a problem hiding this comment.
Yeah, it should better be placed within
connectif possible. How do we use this plugin? I suspect the jar should be provided anyway (?)
I believe there is a way to decide if the plugin is loaded and there is a default way of if this is loaded or not. Let me check that. I am not sure if the default behavior is loading the jar or not loading the jar.
There was a problem hiding this comment.
Moved to connect module.
| private[spark] val CONNECT_GRPC_DEBUG_MODE = | ||
| ConfigBuilder("spark.connect.grpc.debug.enabled") | ||
| .version("3.4.0") | ||
| .booleanConf |
There was a problem hiding this comment.
I deleted this config to make this PR only focus on one thing, which is the configurable port.
This config and what it tries to enable worth a different effort to fully finalize.
core/src/main/scala/org/apache/spark/internal/config/Connect.scala
Outdated
Show resolved
Hide resolved
| private[spark] object Connect { | ||
|
|
||
| private[spark] val CONNECT_GRPC_DEBUG_MODE = | ||
| ConfigBuilder("spark.connect.grpc.debug.enabled") |
There was a problem hiding this comment.
def startGRPCService(): Unit = {
val debugMode = SparkEnv.get.conf.getBoolean("spark.connect.grpc.debug.enabled", true)
This is being called in the grpc service. I was thinking given that this is already define so probably just add this config as well.
There was a problem hiding this comment.
It's used already but I guess this flag and what is tries to enable worth extra work and PR.
I deleted this config to make this PR only focus on one thing.
|
Can one of the admins verify this patch? |
| def startGRPCService(): Unit = { | ||
| val debugMode = SparkEnv.get.conf.getBoolean("spark.connect.grpc.debug.enabled", true) | ||
| val port = 15002 | ||
| val port = SparkEnv.get.conf.getInt("spark.connect.grpc.binding.port", 15002) |
There was a problem hiding this comment.
shall we call SparkEnv.get.conf.get(CONNECT_GRPC_BINDING_PORT)?
There was a problem hiding this comment.
Makes sense! Done!
| def startGRPCService(): Unit = { | ||
| val debugMode = SparkEnv.get.conf.getBoolean("spark.connect.grpc.debug.enabled", true) | ||
| val port = 15002 | ||
| val port = SparkEnv.get.conf.getInt(CONNECT_GRPC_BINDING_PORT.key, 15002) |
There was a problem hiding this comment.
This means we are duplicating the default value 15002 in 2 places. SparkConf.get is a better API to use
def get[T](entry: ConfigEntry[T]): T
There was a problem hiding this comment.
Yes this is what I confused on the getInt API which always ask for a default value, and the get, instead, return a string.
The difference of this get is based on the Entry parameter, which was ignored when I searched the API.
Done.
|
Merged to master. |
6 participants
What changes were proposed in this pull request?
Add
Connectconfig and one connect gRPC config keys:spark.connect.grpc.binding.portas Int type.Why are the changes needed?
Currently Spark Connect gRPC port is hardcoded and we can make it configurable.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Existing UT