ksumit commented on code in PR #384:
URL: https://github.com/apache/incubator-livy/pull/384#discussion_r1099607027
##########
server/src/main/scala/org/apache/livy/server/interactive/InteractiveSessionServlet.scala:
##########
@@ -52,15 +53,23 @@ class InteractiveSessionServlet(
override protected def createSession(req: HttpServletRequest):
InteractiveSession = {
val createRequest = bodyAs[CreateInteractiveRequest](req)
+ val sessionId = sessionManager.nextId();
+
+ // Calling getTimeAsMs just to validate the ttl value
+ if (createRequest.ttl.isDefined) {
+ ClientConf.getTimeAsMs(createRequest.ttl.orNull);
Review Comment:
we don't need to do `orNull` here right?
##########
server/src/main/scala/org/apache/livy/server/SessionServlet.scala:
##########
@@ -131,13 +131,18 @@ abstract class SessionServlet[S <: Session, R <:
RecoveryMetadata](
if (tooManySessions) {
BadRequest(ResponseMessage("Rejected, too many sessions are being
created!"))
} else {
- val session = sessionManager.register(createSession(request))
- // Because it may take some time to establish the session, update the
last activity
- // time before returning the session info to the client.
- session.recordActivity()
- Created(clientSessionView(session, request),
- headers = Map("Location" ->
- (getRequestPathInfo(request) + url(getSession, "id" ->
session.id.toString))))
+ try {
+ val session = sessionManager.register(createSession(request))
+ // Because it may take some time to establish the session, update
the last activity
+ // time before returning the session info to the client.
+ session.recordActivity()
+ Created(clientSessionView(session, request),
+ headers = Map("Location" ->
+ (getRequestPathInfo(request) + url(getSession, "id" ->
session.id.toString))))
+ } catch {
+ case e: IllegalArgumentException =>
+ BadRequest(ResponseMessage("Rejected, Invalid ttl field: " +
e.getMessage))
Review Comment:
Right now, the message may be related to ttl field validation but this
doesn't have to be right? Can we do this instead please?
```
BadRequest(ResponseMessage("Rejected, Reason: " + e.getMessage))
```
##########
server/src/main/scala/org/apache/livy/sessions/SessionManager.scala:
##########
@@ -168,7 +170,12 @@ class SessionManager[S <: Session, R <: RecoveryMetadata :
ClassTag](
false
} else {
val currentTime = System.nanoTime()
- currentTime - session.lastActivity > sessionTimeout
+ var calculatedTimeout = sessionTimeout;
+ if (session.ttl.isDefined) {
+ calculatedTimeout = ClientConf.getTimeAsMs(session.ttl.orNull)
Review Comment:
we don't need `orNull` here right?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
