You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@livy.apache.org by "askhatri (via GitHub)" <gi...@apache.org> on 2023/01/23 11:45:06 UTC

[GitHub] [incubator-livy] askhatri opened a new pull request, #384: [LIVY-968][SERVER] Provide ttl field for a livy session

askhatri opened a new pull request, #384:
URL: https://github.com/apache/incubator-livy/pull/384

   ## What changes were proposed in this pull request?
   
   A session should have a field 'ttl' (Time to live) which once exceeded should get the session killed.This field can be a parameter in the POST /sessions request and then be a part of session object. Livy has a config `livy.server.session.timeout` but this is a global config and cannot be configured for a session. A 'ttl' field can help set this for a session level.
   
   JIRA: https://issues.apache.org/jira/browse/LIVY-968
   
   ## How was this patch tested?
   
   Verified manually by creating interactive session via REST API call in a local Yarn cluster. Also, we have add the new unit tests.
   


-- 
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: commits-unsubscribe@livy.apache.org

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

[GitHub] [incubator-livy] ksumit commented on a diff in pull request #384: [LIVY-968][SERVER] Provide ttl field for a livy session

Posted by "ksumit (via GitHub)" <gi...@apache.org>.

ksumit commented on code in PR #384:
URL: https://github.com/apache/incubator-livy/pull/384#discussion_r1089591529


##########
client-common/src/main/java/org/apache/livy/client/common/ClientConf.java:
##########
@@ -36,7 +36,7 @@
 public abstract class ClientConf<T extends ClientConf>
   implements Iterable<Map.Entry<String, String>> {
 
-  protected Logger LOG = LoggerFactory.getLogger(getClass());
+  protected static Logger LOG = LoggerFactory.getLogger(ClientConf.class);

Review Comment:
   should this be final too?



##########
server/src/main/scala/org/apache/livy/sessions/Session.scala:
##########
@@ -138,6 +138,7 @@ abstract class Session(
     val id: Int,
     val name: Option[String],
     val owner: String,
+    val ttl: Option[String],

Review Comment:
   should we add an auxiliary constructor as well that matches the existing constructor (4 parameters only) and takes the global session timeout as ttl by default? this will help reduce the amount of change in this PR.



##########
server/src/main/scala/org/apache/livy/sessions/SessionManager.scala:
##########
@@ -168,7 +169,9 @@ class SessionManager[S <: Session, R <: RecoveryMetadata : ClassTag](
             false
           } else {
             val currentTime = System.nanoTime()
-            currentTime - session.lastActivity > sessionTimeout
+            val calculatedTimeout =

Review Comment:
   nanoseconds seems too much to me, from my exposure to customer usecases i think ttl in minutes should be enough. curious to know when nanoseconds level of granularity would be useful.
   
   please consider simplifying this further by making this an `Int` or `Long` and avoiding all the parsing and validation logic. for the variable naming, what do you think of `ttlInMinutes` or `ttlInSeconds`?



##########
docs/rest-api.md:
##########
@@ -151,6 +151,11 @@ Creates a new interactive Scala, Python, or R shell in the cluster.
     <td>Timeout in second to which session be orphaned</td>
     <td>int</td>
   </tr>
+  <tr>
+    <td>ttl</td>
+    <td>The timeout for this inactive session</td>

Review Comment:
   would be useful to provide some examples here.
   
   `timeout for this inactive session in minutes/seconds, example: 10`



-- 
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: commits-unsubscribe@livy.apache.org

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

[GitHub] [incubator-livy] lmccay merged pull request #384: [LIVY-968][SERVER] Provide ttl field for a livy session

Posted by "lmccay (via GitHub)" <gi...@apache.org>.

lmccay merged PR #384:
URL: https://github.com/apache/incubator-livy/pull/384


-- 
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: commits-unsubscribe@livy.apache.org

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