ksumit commented on code in PR #384:
URL: https://github.com/apache/incubator-livy/pull/384#discussion_r1097865760
##########
server/src/main/scala/org/apache/livy/server/interactive/InteractiveSessionServlet.scala:
##########
@@ -52,15 +53,18 @@ class InteractiveSessionServlet(
override protected def createSession(req: HttpServletRequest):
InteractiveSession = {
val createRequest = bodyAs[CreateInteractiveRequest](req)
+ val sessionId = sessionManager.nextId();
+ ClientConf.getTimeAsNanos(createRequest.ttl.orNull, sessionId, 0L);
Review Comment:
this could be place for us to parse and set the ttl value correctly:
1. if it's present and valid, parse it and set to the parsed value
2. if it's invalid, set it to the global session timeout value or reject the
request.
This method call is doing the same thing but passing 0L as default value
seems hacky/confusing to me. Also calling a get method in isolation without
using the return value seems hacky/confusing to me.
##########
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 =
+ ClientConf.getTimeAsNanos(session.ttl.orNull, session.id,
sessionTimeout)
Review Comment:
I agree with @gyogal, if the string was validated and converted to valid
long value at session creation, we would not have to worry about these issues
at later stages. If this value is not passed in the request, we can use the
global timeout value from `livy.server.session.timeout` or
`LivyConf.SESSION_TIMEOUT`?
##########
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:
Hey @askhatri, there is a lot of duplicate code in `getTimeAsMs()` and
`getTimeAsNanos()`, here is how i think this can be resolved (please see the
changed methods below, i've merged some of your validation logic as well). My
hope is that we will be able to use `getTimeAsMs()` in this form instead of
`getTimeAsNanos()`.
```
public long getTimeAsMs(ConfEntry e) {
String time = get(e, String.class);
if (time == null) {
check(e.dflt() != null,
"ConfEntry %s doesn't have a default value, cannot convert to time
value.", e.key());
time = (String) e.dflt();
}
return getTimeAsMs(time);
}
public static long getTimeAsMs(String time) {
check(time != null && !time.trim().isEmpty(), "Invalid time string: %s",
time);
Matcher m =
Pattern.compile("(-?[0-9]+)([a-z]+)?").matcher(time.toLowerCase());
if (!m.matches()) {
throw new IllegalArgumentException("Invalid time string: " + time);
}
long val = Long.parseLong(m.group(1));
String suffix = m.group(2);
if (suffix != null && !TIME_SUFFIXES.containsKey(suffix)) {
throw new IllegalArgumentException("Invalid suffix: \"" + suffix +
"\"");
}
return TimeUnit.MILLISECONDS.convert(val,
suffix != null ? TIME_SUFFIXES.get(suffix) : TimeUnit.MILLISECONDS);
}
private static void check(boolean test, String message, Object... args) {
if (!test) {
throw new IllegalArgumentException(String.format(message, args));
}
}
```
--
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]
