[GitHub] [spark] juliuszsompolski commented on a diff in pull request #42221: [SPARK-44595][CONNECT] Make the user session cache number and cache time be configurable in spark connect service

["juliuszsompolski (via GitHub)" <gi...@apache.org>]

juliuszsompolski commented on code in PR #42221:
URL: https://github.com/apache/spark/pull/42221#discussion_r1280016548


##########
connector/connect/server/src/main/scala/org/apache/spark/sql/connect/config/Connect.scala:
##########
@@ -121,4 +121,18 @@ object Connect {
       .version("3.5.0")
       .booleanConf
       .createWithDefault(false)
+
+  val CONNECT_USER_SESSION_CACHE_SIZE =
+    ConfigBuilder("spark.connect.userSession.cacheSize")
+      .doc("Sets the cache size of user session.")
+      .version("4.0.0")
+      .intConf
+      .createWithDefault(100)
+
+  val CONNECT_USER_SESSION_CACHE_TIMEOUT =
+    ConfigBuilder("spark.connect.userSession.cacheTimeout")
+      .doc("Sets the cache timeout of user session.")
+      .version("4.0.0")
+      .intConf
+      .createWithDefault(3600)

Review Comment:
   I am not sure if we want to keep the session cache semantics being like it is now in the long term.
   Because of that, I would not codify these configs into documented public configs, that will later restrict us from changing the behaviour of the cache.
   For example
   - should we have this global limit of how many sessions there are at all? so that a malicious user can flood the server with sessions, and get other people's sessions evicted?
   - should the timeout be 3600 seconds? There is currently no way to close session, so a client that goes away just leaves it hanging for an hour. We should have some mechanism for sessions liveness, like e.g. a heartbeat from client that they are still there, and then likely a timeout that is much lower if the client doesn't go away. But since we need to maintain backwards compatibility with old client, we need to leave it working with 1h timeout for old clients...
   
   I am working on designing these matters, and I think we shouldn't put the current behaviour into user facing comment, so that we maintain flexibility of changing it...
   
   Maybe .internal() for now would be fine, because I see the value in just having something to control it for now?
   
   cc @grundprinzip 



-- 
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: reviews-unsubscribe@spark.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org