[GitHub] tysonnorris commented on a change in pull request #3941: to address #3918, reuse a container on applicationError

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

tysonnorris commented on a change in pull request #3941: to address #3918, reuse a container on applicationError
URL: https://github.com/apache/incubator-openwhisk/pull/3941#discussion_r207710343
 
 

 ##########
 File path: core/invoker/src/main/scala/whisk/core/containerpool/ContainerProxy.scala
 ##########
 @@ -371,57 +371,65 @@ class ContainerProxy(
               val initRunInterval = initInterval
                 .map(i => Interval(runInterval.start.minusMillis(i.duration.toMillis), runInterval.end))
                 .getOrElse(runInterval)
-              ContainerProxy.constructWhiskActivation(job, initInterval, initRunInterval, response)
+              ContainerProxy.constructWhiskActivation(job, initInterval, initRunInterval, response) -> false
           }
       }
       .recover {
         case InitializationError(interval, response) =>
-          ContainerProxy.constructWhiskActivation(job, Some(interval), interval, response)
+          ContainerProxy.constructWhiskActivation(job, Some(interval), interval, response) -> true
         case t =>
           // Actually, this should never happen - but we want to make sure to not miss a problem
           logging.error(this, s"caught unexpected error while running activation: ${t}")
           ContainerProxy.constructWhiskActivation(
             job,
             None,
             Interval.zero,
-            ActivationResponse.whiskError(Messages.abnormalRun))
+            ActivationResponse.whiskError(Messages.abnormalRun)) -> true
       }
 
     // Sending active ack. Entirely asynchronous and not waited upon.
-    activation.foreach(
-      sendActiveAck(tid, _, job.msg.blocking, job.msg.rootControllerIndex, job.msg.user.namespace.uuid))
+    activation.foreach {
+      case (a, _) => sendActiveAck(tid, a, job.msg.blocking, job.msg.rootControllerIndex, job.msg.user.namespace.uuid)
+    }
 
     // Adds logs to the raw activation.
-    val activationWithLogs: Future[Either[ActivationLogReadingError, WhiskActivation]] = activation
-      .flatMap { activation =>
-        // Skips log collection entirely, if the limit is set to 0
-        if (job.action.limits.logs.asMegaBytes == 0.MB) {
-          Future.successful(Right(activation))
-        } else {
-          val start = tid.started(this, LoggingMarkers.INVOKER_COLLECT_LOGS, logLevel = InfoLevel)
-          collectLogs(tid, job.msg.user, activation, container, job.action)
-            .andThen {
-              case Success(_) => tid.finished(this, start)
-              case Failure(t) => tid.failed(this, start, s"reading logs failed: $t")
-            }
-            .map(logs => Right(activation.withLogs(logs)))
-            .recover {
-              case LogCollectingException(logs) =>
-                Left(ActivationLogReadingError(activation.withLogs(logs)))
-              case _ =>
-                Left(ActivationLogReadingError(activation.withLogs(ActivationLogs(Vector(Messages.logFailure)))))
+    val activationWithLogs: Future[Either[ActivationLogReadingError, (WhiskActivation, Boolean)]] = activation
+      .flatMap {
+        {
+          case (activation, initFailure) =>
+            // Skips log collection entirely, if the limit is set to 0
+            if (job.action.limits.logs.asMegaBytes == 0.MB) {
+              Future.successful(Right(activation -> initFailure))
+            } else {
+              val start = tid.started(this, LoggingMarkers.INVOKER_COLLECT_LOGS, logLevel = InfoLevel)
+              collectLogs(tid, job.msg.user, activation, container, job.action)
+                .andThen {
+                  case Success(_) => tid.finished(this, start)
+                  case Failure(t) => tid.failed(this, start, s"reading logs failed: $t")
+                }
+                .map(logs => Right(activation.withLogs(logs) -> initFailure))
+                .recover {
+                  case LogCollectingException(logs) =>
+                    Left(ActivationLogReadingError(activation.withLogs(logs)))
+                  case _ =>
+                    Left(ActivationLogReadingError(activation.withLogs(ActivationLogs(Vector(Messages.logFailure)))))
+                }
             }
         }
+
       }
 
     // Storing the record. Entirely asynchronous and not waited upon.
-    activationWithLogs.map(_.fold(_.activation, identity)).foreach(storeActivation(tid, _))
+    activationWithLogs.map(_.fold(_.activation, _._1)).foreach(storeActivation(tid, _))
 
     // Disambiguate activation errors and transform the Either into a failed/successful Future respectively.
     activationWithLogs.flatMap {
-      case Right(act) if !act.response.isSuccess => Future.failed(ActivationUnsuccessfulError(act))
-      case Left(error)                           => Future.failed(error)
-      case Right(act)                            => Future.successful(act)
+      //if non-successful, init failures should fail, and non-applicationErrors should also fail
+      case Right((act, initFailure)) if !act.response.isSuccess && (initFailure || !act.response.isApplicationError) =>
 
 Review comment:
   Sounds reasonable! 👍 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on 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