[GitHub] [incubator-livy] Ngone51 commented on a change in pull request #166: [LIVY-585][SERVER] Avoid duplicate stopping log when stop/interrupt a session

[GitBox <gi...@apache.org>]

Ngone51 commented on a change in pull request #166: [LIVY-585][SERVER] Avoid duplicate stopping log when stop/interrupt a session
URL: https://github.com/apache/incubator-livy/pull/166#discussion_r279144528
 
 

 ##########
 File path: server/src/main/scala/org/apache/livy/sessions/Session.scala
 ##########
 @@ -184,30 +185,32 @@ abstract class Session(
   def start(): Unit
 
   def stop(): Future[Unit] = Future {
-    try {
-      info(s"Stopping $this...")
-      stopSession()
-      info(s"Stopped $this.")
-    } catch {
-      case e: Exception =>
-        warn(s"Error stopping session $id.", e)
-    }
+    if (_stopped.compareAndSet(false, true)) {
 
 Review comment:
   > What about using the `state`? 
   
   Previously, I used `state`, which probably likes *if the `state` is already `FinishedSessionState`, do not do stop again*. But I found there's code(see below) which transit `state` to `FinishedSessionState` first before calling stop(). So, this strategy can not work, then I try to use atomic boolean here.
   
   ```
   if (serverSideState != SessionState.ShuttingDown) {
     transition(SessionState.Error())
     stop()
     app.foreach { a =>
        info(s"Failed to ping RSC driver for session $id. Killing application.")
           a.kill()
        }
   }
   ```
   
   > we might want to re-try to stop it, and this doesn't allow that..
   
   Yeah, this can be a problem if we want to retry... Do we have some retry mechanism in some place by now ?

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services