You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@livy.apache.org by "jianzhenwu (via GitHub)" <gi...@apache.org> on 2023/08/25 09:28:31 UTC
[GitHub] [incubator-livy] jianzhenwu opened a new pull request, #416: [LIVY-987] NPE when waiting for thrift session to start timeout.
jianzhenwu opened a new pull request, #416:
URL: https://github.com/apache/incubator-livy/pull/416
## 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
Please review https://livy.incubator.apache.org/community/ before opening a pull request.
--
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
[GitHub] [incubator-livy] mgaido91 commented on pull request #416: [LIVY-987] NPE when waiting for thrift session to start timeout.
Posted by "mgaido91 (via GitHub)" <gi...@apache.org>.
mgaido91 commented on PR #416:
URL: https://github.com/apache/incubator-livy/pull/416#issuecomment-1693155712
Can we add a UT to ensure that such problem will not appear again in the future?
--
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
[GitHub] [incubator-livy] jianzhenwu commented on pull request #416: [LIVY-987] NPE when waiting for thrift session to start timeout.
Posted by "jianzhenwu (via GitHub)" <gi...@apache.org>.
jianzhenwu commented on PR #416:
URL: https://github.com/apache/incubator-livy/pull/416#issuecomment-1704563058
> Looks good, only a question and a minor style change
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,
--
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
[GitHub] [incubator-livy] codecov-commenter commented on pull request #416: [LIVY-987] NPE when waiting for thrift session to start timeout.
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #416:
URL: https://github.com/apache/incubator-livy/pull/416#issuecomment-1700306940
## [Codecov](https://app.codecov.io/gh/apache/incubator-livy/pull/416?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#416](https://app.codecov.io/gh/apache/incubator-livy/pull/416?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (c2ad37a) into [master](https://app.codecov.io/gh/apache/incubator-livy/commit/5dccc479c6087112f048a7e5cff0723855ef14e9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (5dccc47) will **decrease** coverage by `36.92%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #416 +/- ##
=============================================
- Coverage 65.50% 28.58% -36.92%
+ Complexity 952 378 -574
=============================================
Files 103 103
Lines 6062 6062
Branches 916 916
=============================================
- Hits 3971 1733 -2238
- Misses 1542 3976 +2434
+ Partials 549 353 -196
```
[see 86 files with indirect coverage changes](https://app.codecov.io/gh/apache/incubator-livy/pull/416/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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
[GitHub] [incubator-livy] mgaido91 commented on pull request #416: [LIVY-987] NPE when waiting for thrift session to start timeout.
Posted by "mgaido91 (via GitHub)" <gi...@apache.org>.
mgaido91 commented on PR #416:
URL: https://github.com/apache/incubator-livy/pull/416#issuecomment-1704926482
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
--
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
[GitHub] [incubator-livy] jianzhenwu commented on pull request #416: [LIVY-987] NPE when waiting for thrift session to start timeout.
Posted by "jianzhenwu (via GitHub)" <gi...@apache.org>.
jianzhenwu commented on PR #416:
URL: https://github.com/apache/incubator-livy/pull/416#issuecomment-1695019113
> 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.
--
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
[GitHub] [incubator-livy] mgaido91 commented on a diff in pull request #416: [LIVY-987] NPE when waiting for thrift session to start timeout.
Posted by "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
[GitHub] [incubator-livy] mgaido91 commented on pull request #416: [LIVY-987] NPE when waiting for thrift session to start timeout.
Posted by "mgaido91 (via GitHub)" <gi...@apache.org>.
mgaido91 commented on PR #416:
URL: https://github.com/apache/incubator-livy/pull/416#issuecomment-1704848857
LGTM
--
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
[GitHub] [incubator-livy] mgaido91 commented on a diff in pull request #416: [LIVY-987] NPE when waiting for thrift session to start timeout.
Posted by "mgaido91 (via GitHub)" <gi...@apache.org>.
mgaido91 commented on code in PR #416:
URL: https://github.com/apache/incubator-livy/pull/416#discussion_r1313767837
##########
thriftserver/server/src/test/scala/org/apache/livy/thriftserver/TestLivyThriftSessionManager.scala:
##########
@@ -49,6 +58,17 @@ class TestLivyThriftSessionManager {
}
conf.set(entry, limit)
}
+ this.createThriftSessionManager(conf)
+ }
+ private def createThriftSessionManager(
Review Comment:
```suggestion
private def createThriftSessionManager(
```
##########
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:
could you please provide an explanation here?
--
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
[GitHub] [incubator-livy] mgaido91 closed pull request #416: [LIVY-987] NPE when waiting for thrift session to start timeout.
Posted by "mgaido91 (via GitHub)" <gi...@apache.org>.
mgaido91 closed pull request #416: [LIVY-987] NPE when waiting for thrift session to start timeout.
URL: https://github.com/apache/incubator-livy/pull/416
--
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