[GitHub] [incubator-livy] mgaido91 commented on a diff in pull request #416: [LIVY-987] NPE when waiting for thrift session to start timeout.

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

mgaido91 commented on code in PR #416:
URL: https://github.com/apache/incubator-livy/pull/416#discussion_r1310372309


##########
thriftserver/server/src/test/scala/org/apache/livy/thriftserver/TestLivyThriftSessionManager.scala:
##########
@@ -90,7 +100,7 @@ class TestLivyThriftSessionManager {
 
   @Test
   def testLimitConnectionsByIpAddress(): Unit = {
-    val thriftSessionMgr = createThriftSessionManager(IpAddress)
+    val thriftSessionMgr = createThriftSessionManager(None, IpAddress)

Review Comment:
   if we create an overloaded method, we can avoid these changes



##########
thriftserver/server/src/test/scala/org/apache/livy/thriftserver/TestLivyThriftSessionManager.scala:
##########
@@ -36,11 +45,12 @@ class TestLivyThriftSessionManager {
 
   import ConnectionLimitType._
 
-  private def createThriftSessionManager(
+  private def createThriftSessionManager(maxSessionWait: Option[String],
       limitTypes: ConnectionLimitType*): LivyThriftSessionManager = {

Review Comment:
   I think it would be great if we could create an overloaded method where we can pass the `LivyConf`. Might be useful in the future as well.
   
   
   Btw, regarding style, it should be 
   
   ```
     private def createThriftSessionManager(
         maxSessionWait: Option[String], limitTypes: ConnectionLimitType*): LivyThriftSessionManager = {
   ```
   
   or 
   
   ```
     private def createThriftSessionManager(
         maxSessionWait: Option[String],
         limitTypes: ConnectionLimitType*): LivyThriftSessionManager = {
   ```
   
   if it is too long



##########
thriftserver/server/src/test/scala/org/apache/livy/thriftserver/TestLivyThriftSessionManager.scala:
##########
@@ -142,4 +152,28 @@ class TestLivyThriftSessionManager {
     val msg = s"Connection limit per user reached (user: $user limit: 3)"
     testLimit(thriftSessionMgr, user, ipAddress, forwardedAddresses, msg)
   }
+
+  @Test(expected = classOf[TimeoutException])
+  def testGetLivySessionWaitForTimeout(): Unit = {
+    val thriftSessionMgr = createThriftSessionManager(Some("10ms"))
+    val sessionHandle = mock(classOf[SessionHandle])
+    val future = Future[InteractiveSession] {
+      sleep(100)
+      mock(classOf[InteractiveSession])
+    }
+    thriftSessionMgr._mockLivySession(sessionHandle, future)
+    thriftSessionMgr.getLivySession(sessionHandle)
+  }
+
+  @Test(expected = classOf[TimeoutException])
+  def testGetLivySessionWithTimeoutException(): Unit = {

Review Comment:
   what does this test check? What does it add to the test above?



##########
thriftserver/server/src/main/scala/org/apache/livy/thriftserver/LivyThriftSessionManager.scala:
##########
@@ -549,6 +549,11 @@ class LivyThriftSessionManager(val server: LivyThriftServer, val livyConf: LivyC
   def getSessionInfo(sessionHandle: SessionHandle): SessionInfo = {
     sessionInfo.get(sessionHandle)
   }
+
+  private[thriftserver] def _mockLivySession(

Review Comment:
   shall we rather make `sessionHandleToLivySession` visible by `private[thriftserver]` so we can avoid adding this method only for testing? Why do you think this option is better?



-- 
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