[GitHub] [openwhisk] rabbah commented on a change in pull request #4954: Use latest code if action's revision is mismatched

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

rabbah commented on a change in pull request #4954:
URL: https://github.com/apache/openwhisk/pull/4954#discussion_r501033987



##########
File path: core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/InvokerReactive.scala
##########
@@ -179,58 +234,7 @@ class InvokerReactive(
 
         if (!namespaceBlacklist.isBlacklisted(msg.user)) {
           val start = transid.started(this, LoggingMarkers.INVOKER_ACTIVATION, logLevel = InfoLevel)
-          val namespace = msg.action.path
-          val name = msg.action.name
-          val actionid = FullyQualifiedEntityName(namespace, name).toDocId.asDocInfo(msg.revision)
-          val subject = msg.user.subject
-
-          logging.debug(this, s"${actionid.id} $subject ${msg.activationId}")
-
-          // caching is enabled since actions have revision id and an updated
-          // action will not hit in the cache due to change in the revision id;
-          // if the doc revision is missing, then bypass cache
-          if (actionid.rev == DocRevision.empty) logging.warn(this, s"revision was not provided for ${actionid.id}")
-
-          WhiskAction
-            .get(entityStore, actionid.id, actionid.rev, fromCache = actionid.rev != DocRevision.empty)
-            .flatMap { action =>
-              action.toExecutableWhiskAction match {
-                case Some(executable) =>
-                  pool ! Run(executable, msg)
-                  Future.successful(())
-                case None =>
-                  logging.error(this, s"non-executable action reached the invoker ${action.fullyQualifiedName(false)}")
-                  Future.failed(new IllegalStateException("non-executable action reached the invoker"))
-              }
-            }
-            .recoverWith {
-              case t =>
-                // If the action cannot be found, the user has concurrently deleted it,

Review comment:
       This comment is still valid - why was it removed?

##########
File path: core/invoker/src/main/scala/org/apache/openwhisk/core/invoker/InvokerReactive.scala
##########
@@ -164,6 +164,62 @@ class InvokerReactive(
   private val pool =
     actorSystem.actorOf(ContainerPool.props(childFactory, poolConfig, activationFeed, prewarmingConfigs))
 
+  def handleActivationMessage(msg: ActivationMessage)(implicit transid: TransactionId): Future[Unit] = {
+    val namespace = msg.action.path
+    val name = msg.action.name
+    val actionid = FullyQualifiedEntityName(namespace, name).toDocId.asDocInfo(msg.revision)
+    val subject = msg.user.subject
+
+    logging.debug(this, s"${actionid.id} $subject ${msg.activationId}")
+
+    // caching is enabled since actions have revision id and an updated
+    // action will not hit in the cache due to change in the revision id;
+    // if the doc revision is missing, then bypass cache
+    if (actionid.rev == DocRevision.empty) logging.warn(this, s"revision was not provided for ${actionid.id}")
+
+    WhiskAction
+      .get(entityStore, actionid.id, actionid.rev, fromCache = actionid.rev != DocRevision.empty)
+      .flatMap(action => {
+        action.toExecutableWhiskAction match {
+          case Some(executable) =>
+            val newMsg = msg.copy(revision = executable.rev, action = action.fullyQualifiedName(true))
+            pool ! Run(executable, newMsg)
+            Future.successful(())
+          case None =>
+            logging.error(this, s"non-executable action reached the invoker ${action.fullyQualifiedName(false)}")
+            Future.failed(new IllegalStateException("non-executable action reached the invoker"))
+        }
+      })
+      .recoverWith {
+        case DocumentRevisionMismatchException(_) =>
+          // if revision is mismatched, the action may have been updated,
+          // so try again with the latest code
+          handleActivationMessage(msg.copy(revision = DocRevision.empty))

Review comment:
       Since you're also working on versioning - I think this change should be considered in that broader context.
   
   If the sequence or composition references actions specifically by version, then it should be an error to invoke an alternate version. If the sequence uses "latest" then this change is acceptable.
   
   Was the intent for this change strictly to address compositions? 




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