[GitHub] [openwhisk] markusthoemmes commented on a change in pull request #4971: cleaning up logic and test issues with ContainerProxy

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

markusthoemmes commented on a change in pull request #4971:
URL: https://github.com/apache/openwhisk/pull/4971#discussion_r485353391



##########
File path: core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerProxy.scala
##########
@@ -697,7 +698,7 @@ class ContainerProxy(factory: (TransactionId,
 
   def abortBuffered(abortResponse: Option[ActivationResponse] = None) = {

Review comment:
       Can we add an explicit return type here to make sure we return a Future?

##########
File path: core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/ContainerProxy.scala
##########
@@ -714,14 +715,19 @@ class ContainerProxy(factory: (TransactionId,
       } else {
         CompletionMessage(tid, result, instance)
       }
-      sendActiveAck(tid, result, job.msg.blocking, job.msg.rootControllerIndex, job.msg.user.namespace.uuid, msg)
-        .andThen {
-          case Failure(e) => logging.error(this, s"failed to send abort ack $e")
-        }
-      storeActivation(tid, result, job.msg.blocking, context).andThen {
-        case Failure(e) => logging.error(this, s"failed to store aborted activation $e")
+      val ack =
+        sendActiveAck(tid, result, job.msg.blocking, job.msg.rootControllerIndex, job.msg.user.namespace.uuid, msg)
+          .recover {

Review comment:
       Do you really want to recover (make the Future succeed) here rather than just log? Same below. The sequence below will always succeed.




----------------------------------------------------------------
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