ksumit commented on code in PR #384:
URL: https://github.com/apache/incubator-livy/pull/384#discussion_r1093693598
##########
client-common/src/main/java/org/apache/livy/client/common/ClientConf.java:
##########
@@ -265,6 +265,51 @@ private Map<String, ConfPair> allAlternativeKeys() {
return altToNewKeyMap;
}
+ public static boolean validateTtl(String ttl) {
Review Comment:
Looks like `validateTtl()` and `getTimeAsNanos()` are using a lot of
duplicate code, could we avoid this?
##########
server/src/main/scala/org/apache/livy/server/interactive/InteractiveSessionServlet.scala:
##########
@@ -52,15 +53,20 @@ class InteractiveSessionServlet(
override protected def createSession(req: HttpServletRequest):
InteractiveSession = {
val createRequest = bodyAs[CreateInteractiveRequest](req)
- InteractiveSession.create(
- sessionManager.nextId(),
- createRequest.name,
- remoteUser(req),
- proxyUser(req, createRequest.proxyUser),
- livyConf,
- accessManager,
- createRequest,
- sessionStore)
+ if (ClientConf.validateTtl(createRequest.ttl.orNull)) {
+ InteractiveSession.create(
+ sessionManager.nextId(),
+ createRequest.name,
+ remoteUser(req),
+ proxyUser(req, createRequest.proxyUser),
+ livyConf,
+ accessManager,
+ createRequest,
+ sessionStore,
+ createRequest.ttl)
+ } else {
+ null
Review Comment:
should this be considered an error case instead and we throw an appropriate
error message here? Or could we log the failure and use the global ttl value in
this case as the default value?
##########
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:
I see now that global configuration is also using this complicated logic.
Thanks for pointing that out. I also noticed that there are bunch of other
configurations that follow this same logic. So we can leave this as it is. I'm
concerned about duplicate code logic in `getTimeAsMs()` and `getTimeAsNanos()`,
please see if we can avoid the same or reuse what's already there?
##########
server/src/main/scala/org/apache/livy/server/interactive/CreateInteractiveRequest.scala:
##########
@@ -50,6 +51,7 @@ class CreateInteractiveRequest {
(if (queue.isDefined) s"queue: ${queue.get}, " else "") +
(if (name.isDefined) s"name: ${name.get}, " else "") +
(if (conf.nonEmpty) s"conf: ${conf.mkString(",")}, " else "") +
- s"heartbeatTimeoutInSecond: $heartbeatTimeoutInSecond]"
+ s"heartbeatTimeoutInSecond: $heartbeatTimeoutInSecond, " +
+ (if (ttl.isDefined) s"driverMemory: ${ttl.get}]" else "]")
Review Comment:
typo: we need `ttl` instead of `driverMemory`
##########
client-common/src/main/java/org/apache/livy/client/common/HttpMessages.java:
##########
@@ -61,9 +61,10 @@ public static class SessionInfo implements ClientMessage {
public final String kind;
public final Map<String, String> appInfo;
public final List<String> log;
+ public final String ttl;
public SessionInfo(int id, String name, String appId, String owner, String
proxyUser,
Review Comment:
given that ttl is optional field, should we create an override constructor
without this parameter? that way you will be able to avoid other changes in
test classes.
##########
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))))
+ val input = createSession(request)
+ if(input == null) {
+ BadRequest(ResponseMessage("Rejected, invalid value for ttl field!"))
Review Comment:
should we throw an exception in `createSession()` and use appropriate
message from that to return the `ResponseMessage` here? There could potentially
be other reasons on why `createSession()` could return null, right?
##########
client-common/src/main/java/org/apache/livy/client/common/ClientConf.java:
##########
@@ -265,6 +265,51 @@ private Map<String, ConfPair> allAlternativeKeys() {
return altToNewKeyMap;
}
+ public static boolean validateTtl(String ttl) {
+ if (ttl == null || ttl.trim().isEmpty()) {
+ return true;
+ }
+ Matcher m =
Pattern.compile("(-?[0-9]+)([a-z]+)?").matcher(ttl.trim().toLowerCase());
+ if (!m.matches()) {
+ LOG.error("Invalid ttl string: " + ttl);
+ return false;
+ }
+ long val = Long.parseLong(m.group(1));
+ if (val <= 0L) {
+ LOG.error("Invalid ttl value: " + val);
+ return false;
+ }
+ String suffix = m.group(2);
+ if (suffix != null && !TIME_SUFFIXES.containsKey(suffix)) {
+ LOG.error("Invalid ttl suffix: " + suffix);
+ return false;
+ }
+ return true;
+ }
+
+ public static long getTimeAsNanos(String time, int sessionId, long
defaultVale) {
Review Comment:
What is your opinion in reusing `getTimeAsMs()` instead of creating a new
one? Other than the granularity, do you see any other gap between
`getTimeAsMs()` and `getTimeAsNanos()` implementations?
--
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]
