[LIVY-987] NPE when waiting for thrift session to start timeout. #416
Conversation
|
Can we add a UT to ensure that such problem will not appear again in the future? |
thank you for your reply. Of course, please review again. |
| private def createThriftSessionManager(maxSessionWait: Option[String], | ||
| limitTypes: ConnectionLimitType*): LivyThriftSessionManager = { |
There was a problem hiding this 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
...server/server/src/test/scala/org/apache/livy/thriftserver/TestLivyThriftSessionManager.scala
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Test(expected = classOf[TimeoutException]) | ||
| def testGetLivySessionWithTimeoutException(): Unit = { |
There was a problem hiding this comment.
what does this test check? What does it add to the test above?
There was a problem hiding this comment.
could you please provide an explanation here?
| @@ -549,6 +549,11 @@ class LivyThriftSessionManager(val server: LivyThriftServer, val livyConf: LivyC | |||
| def getSessionInfo(sessionHandle: SessionHandle): SessionInfo = { | |||
| sessionInfo.get(sessionHandle) | |||
| } | |||
|
|
|||
| private[thriftserver] def _mockLivySession( | |||
There was a problem hiding this 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?
Codecov Report
@@ Coverage Diff @@
## master #416 +/- ##
=============================================
- Coverage 65.50% 28.68% -36.82%
+ Complexity 952 379 -573
=============================================
Files 103 103
Lines 6062 6062
Branches 916 916
=============================================
- Hits 3971 1739 -2232
- Misses 1542 3971 +2429
+ Partials 549 352 -197 see 87 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
...server/server/src/test/scala/org/apache/livy/thriftserver/TestLivyThriftSessionManager.scala
Show resolved
Hide resolved
| } | ||
|
|
||
| @Test(expected = classOf[TimeoutException]) | ||
| def testGetLivySessionWithTimeoutException(): Unit = { |
There was a problem hiding this comment.
could you please provide an explanation here?
…r/TestLivyThriftSessionManager.scala Co-authored-by: Marco Gaido <[email protected]>
My point is that this test is used to cover the case where Furetur#isCompleted is true. This may be a bit redundant. If you suggest removing it, I can add a commit to this PR to remove it, |
|
LGTM |
|
I am having some problems to merge this as the current script requires Python2 while I have only python3 on my M1 OSX and installing python 2 is not easy. We should probably migrate the merge script to python3. I will try and do that in the next days, but if someone else wants to merge this, LGTM |
## What changes were proposed in this pull request? Fix NPE when waiting for thrift session to start timeout. https://issues.apache.org/jira/browse/LIVY-987 ## How was this patch tested? manual tests by beeline and set timeout to 10s. 0: jdbc:hive2://username:passwordthrift-server> select 123; RSC client is executing SQL query: select 123, statementId = 681bc017-8f37-4665-a575-da355db77254, session = SessionHandle [c17f1729-6ee1-4260-b82b-aebec3b08e14] Livy session has not yet started. Please wait for it to be ready... Error: java.util.concurrent.TimeoutException: Futures timed out after [10000 milliseconds] (state=,code=0) 0: jdbc:hive2://username:passwordthrift-server> Closing: 0: jdbc:hive2://username:passwordthrift-server Please review https://livy.incubator.apache.org/community/ before opening a pull request. Author: jianzhen.wu <[email protected]> Author: jianzhenwu <[email protected]> Closes apache#416 from jianzhenwu/LIVY-987.
What changes were proposed in this pull request?
Fix NPE when waiting for thrift session to start timeout.
https://issues.apache.org/jira/browse/LIVY-987
How was this patch tested?
manual tests by beeline and set timeout to 10s.
0: jdbc:hive2://username:password@thrift-server> select 123;
RSC client is executing SQL query: select 123, statementId = 681bc017-8f37-4665-a575-da355db77254, session = SessionHandle [c17f1729-6ee1-4260-b82b-aebec3b08e14]
Livy session has not yet started. Please wait for it to be ready...
Error: java.util.concurrent.TimeoutException: Futures timed out after [10000 milliseconds] (state=,code=0)
0: jdbc:hive2://username:password@thrift-server> Closing: 0: jdbc:hive2://username:password@thrift-server
Please review https://livy.incubator.apache.org/community/ before opening a pull request.