You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kyuubi.apache.org by GitBox <gi...@apache.org> on 2021/07/18 03:26:56 UTC

[GitHub] [incubator-kyuubi] wuchunfu opened a new pull request #822: [KYUUBI 821] Improper exception for closing a non-existent session

wuchunfu opened a new pull request #822:
URL: https://github.com/apache/incubator-kyuubi/pull/822


   
   fix #821
   
   ### _How was this patch tested?_
   - [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible
   
   - [ ] Add screenshots for manual tests if appropriate
   
   - [ ] [Run test](https://kyuubi.readthedocs.io/en/latest/tools/testing.html#running-tests) locally before make 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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #822: [KYUUBI 821] Improper exception for closing a non-existent session

Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #822:
URL: https://github.com/apache/incubator-kyuubi/pull/822#discussion_r671782025



##########
File path: kyuubi-common/src/main/scala/org/apache/kyuubi/session/SessionManager.scala
##########
@@ -67,7 +67,7 @@ abstract class SessionManager(name: String) extends CompositeService(name) {
     _latestLogoutTime = System.currentTimeMillis()
     val session = handleToSession.remove(sessionHandle)
     if (session == null) {
-      throw KyuubiSQLException(s"Invalid $sessionHandle")
+      warn(s"Invalid $sessionHandle, Failed closing session")

Review comment:
       please avoid npe




-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] wuchunfu closed pull request #822: [KYUUBI #821] Improper exception for closing a non-existent session

Posted by GitBox <gi...@apache.org>.
wuchunfu closed pull request #822:
URL: https://github.com/apache/incubator-kyuubi/pull/822


   


-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] wuchunfu commented on a change in pull request #822: [KYUUBI 821] Improper exception for closing a non-existent session

Posted by GitBox <gi...@apache.org>.
wuchunfu commented on a change in pull request #822:
URL: https://github.com/apache/incubator-kyuubi/pull/822#discussion_r671960454



##########
File path: kyuubi-common/src/main/scala/org/apache/kyuubi/session/SessionManager.scala
##########
@@ -66,11 +66,11 @@ abstract class SessionManager(name: String) extends CompositeService(name) {
   def closeSession(sessionHandle: SessionHandle): Unit = {
     _latestLogoutTime = System.currentTimeMillis()
     val session = handleToSession.remove(sessionHandle)
-    if (session == null) {
-      throw KyuubiSQLException(s"Invalid $sessionHandle")
+    if (session != null) {
+      session.close()
+    } else {
+      warn(s"$sessionHandle is closed, current opening sessions $getOpenSessionCount")

Review comment:
       Sorry, I don't know how to fix it. Please give me some guidance.




-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-kyuubi] yaooqinn commented on a change in pull request #822: [KYUUBI 821] Improper exception for closing a non-existent session

Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #822:
URL: https://github.com/apache/incubator-kyuubi/pull/822#discussion_r671943706



##########
File path: kyuubi-common/src/main/scala/org/apache/kyuubi/session/SessionManager.scala
##########
@@ -66,11 +66,11 @@ abstract class SessionManager(name: String) extends CompositeService(name) {
   def closeSession(sessionHandle: SessionHandle): Unit = {
     _latestLogoutTime = System.currentTimeMillis()
     val session = handleToSession.remove(sessionHandle)
-    if (session == null) {
-      throw KyuubiSQLException(s"Invalid $sessionHandle")
+    if (session != null) {
+      session.close()
+    } else {
+      warn(s"$sessionHandle is closed, current opening sessions $getOpenSessionCount")

Review comment:
       this belongs to if-branch




-- 
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@kyuubi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org