You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@openwhisk.apache.org by GitBox <gi...@apache.org> on 2020/08/28 02:11:29 UTC
[GitHub] [openwhisk] upgle opened a new pull request #4954: Use latest code if action's revision is mismatched
upgle opened a new pull request #4954:
URL: https://github.com/apache/openwhisk/pull/4954
<!--- Provide a concise summary of your changes in the Title -->
## Description
If the action is changed while the sequence action is executing, the old revision is referred to when the sub action is executed.
At this time, if the server returns the latest revision, then a boxed error occurs due to an assert condition.
```scala
assert(doc.rev.rev == null || doc.rev.rev == responseRev, "Returned revision should match original argument")
```
To avoid this error, I've updated the code to retry with the latest code if the revision is different.
## Related issue and scope
- https://github.com/apache/openwhisk/issues/4953
## My changes affect the following components
- Invoker
## Types of changes
<!--- What types of changes does your code introduce? Use `x` in all the boxes that apply: -->
- Bug fix (generally a non-breaking change which closes an issue).
## Checklist:
<!--- Please review the points below which help you make sure you've covered all aspects of the change you're making. -->
- [x] I signed an [Apache CLA](https://github.com/apache/openwhisk/blob/master/CONTRIBUTING.md).
- [x] I reviewed the [style guides](https://github.com/apache/openwhisk/wiki/Contributing:-Git-guidelines#code-readiness) and followed the recommendations (Travis CI will check :).
- [x] I added tests to cover my changes.
- [x] My changes require further changes to the documentation.
- [x] I updated the documentation where necessary.
----------------------------------------------------------------
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
[GitHub] [openwhisk] upgle commented on a change in pull request #4954: Use latest code if action's revision is mismatched
Posted by GitBox <gi...@apache.org>.
upgle commented on a change in pull request #4954:
URL: https://github.com/apache/openwhisk/pull/4954#discussion_r478805450
##########
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:
Thank you for your review and I understand your concern.
> For example, if any activation sent to Kafka arrives at the invoker side late, it could happen.
In such a case the activation is intended for the old codes but the latest code will be invoked with this change.
Currently, openwhisk doesn't have a versioning feature. So I don't think there's any problem with always serving the latest code if the DB can't fetch the older version, because action developers are responsible for compatibility between old and new action codes.
HDYT?
----------------------------------------------------------------
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
[GitHub] [openwhisk] style95 commented on a change in pull request #4954: Use latest code if action's revision is mismatched
Posted by GitBox <gi...@apache.org>.
style95 commented on a change in pull request #4954:
URL: https://github.com/apache/openwhisk/pull/4954#discussion_r478800183
##########
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:
This can be controversial but I think the system should take care of this.
This is to handle the case where the underlying actions are updated while a sequence action is invoked.
If we consider the sequence as just a coordinator for underlying actions, it would be fine for it to always invoke the latest code. Once a sequence action is defined with some actions, we don't need to update the sequence action whenever the underlying actions are updated. It means the sequence action itself does not care about the version of actions in it but just focuses on the relation and the execution order of them.
So I think it is reasonable to invoke the latest codes all the time.
And regarding the implementation, while this is great, can we differentiate the sequence case with the others?
I feel like there can be some side effects.
For example, if any activation sent to Kafka arrives at the invoker side late, it could happen.
In such a case the activation is intended for the old codes but the latest code will be invoked with this change.
I did not look into code deeply yet, but can we make the controller setup subsequent activations with the latest codes while invoking a sequence action?
----------------------------------------------------------------
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
[GitHub] [openwhisk] rabbah commented on a change in pull request #4954: Use latest code if action's revision is mismatched
Posted by 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
[GitHub] [openwhisk] upgle commented on a change in pull request #4954: Use latest code if action's revision is mismatched
Posted by GitBox <gi...@apache.org>.
upgle commented on a change in pull request #4954:
URL: https://github.com/apache/openwhisk/pull/4954#discussion_r478805450
##########
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:
Thank you for your review and I understand your concern.
> For example, if any activation sent to Kafka arrives at the invoker side late, it could happen.
In such a case the activation is intended for the old codes but the latest code will be invoked with this change.
Currently, Openwhisk doesn't have a versioning feature. So I don't think there's any problem with always serving the latest code if the DB can't fetch the older version, because action developers are responsible for compatibility between old and new action codes.
HDYT?
----------------------------------------------------------------
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
[GitHub] [openwhisk] style95 merged pull request #4954: Use latest code if action's revision is mismatched
Posted by GitBox <gi...@apache.org>.
style95 merged pull request #4954:
URL: https://github.com/apache/openwhisk/pull/4954
----------------------------------------------------------------
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
[GitHub] [openwhisk] style95 commented on a change in pull request #4954: Use latest code if action's revision is mismatched
Posted by GitBox <gi...@apache.org>.
style95 commented on a change in pull request #4954:
URL: https://github.com/apache/openwhisk/pull/4954#discussion_r496379568
##########
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:
Can we leave at least a warning log to describe the system takes a fallback to the latest codes because of the revision mismatch?
----------------------------------------------------------------
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
[GitHub] [openwhisk] style95 commented on a change in pull request #4954: Use latest code if action's revision is mismatched
Posted by GitBox <gi...@apache.org>.
style95 commented on a change in pull request #4954:
URL: https://github.com/apache/openwhisk/pull/4954#discussion_r501405773
##########
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:
Yes.
That needs to be considered in the action versioning feature I think.
CC: @jiangpengcheng
----------------------------------------------------------------
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
[GitHub] [openwhisk] style95 commented on a change in pull request #4954: Use latest code if action's revision is mismatched
Posted by GitBox <gi...@apache.org>.
style95 commented on a change in pull request #4954:
URL: https://github.com/apache/openwhisk/pull/4954#discussion_r478800183
##########
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:
This can be controversial but I think the system should take care of this.
This is to handle the case where the underlying actions are updated while a sequence action is being invoked.
If we consider the sequence as just a coordinator for underlying actions, it would be fine for it to always invoke the latest code. Once a sequence action is defined with some actions, we don't need to update the sequence action whenever the underlying actions are updated. It means the sequence action itself does not care about the version of actions in it but just focuses on the relation and the execution order of them.
So I think it is reasonable to invoke the latest codes all the time.
And regarding the implementation, while this is great, can we differentiate the sequence case with the others?
I feel like there can be some side effects.
For example, if any activation sent to Kafka arrives at the invoker side late, it could happen.
In such a case the activation is intended for the old codes but the latest code will be invoked with this change.
I did not look into code deeply yet, but can we make the controller setup subsequent activations with the latest codes while invoking a sequence action?
----------------------------------------------------------------
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
[GitHub] [openwhisk] style95 commented on a change in pull request #4954: Use latest code if action's revision is mismatched
Posted by GitBox <gi...@apache.org>.
style95 commented on a change in pull request #4954:
URL: https://github.com/apache/openwhisk/pull/4954#discussion_r496379568
##########
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:
Can we leave at least a warning log to describe the system fallbacks to the latest codes because of the revision mismatch?
----------------------------------------------------------------
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
[GitHub] [openwhisk] style95 commented on a change in pull request #4954: Use latest code if action's revision is mismatched
Posted by GitBox <gi...@apache.org>.
style95 commented on a change in pull request #4954:
URL: https://github.com/apache/openwhisk/pull/4954#discussion_r480019902
##########
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:
Not quite sure about this.
It would be great to listen to other reviewers' opinions as well.
From some point of view, it could be a semantic change as activations that are supposed to be rejected would be invoked successfully.
(With an assumption that the codes are backward compatible.)
How about sending an email to the dev list to get more people to attend this PR?
----------------------------------------------------------------
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
[GitHub] [openwhisk] upgle commented on a change in pull request #4954: Use latest code if action's revision is mismatched
Posted by GitBox <gi...@apache.org>.
upgle commented on a change in pull request #4954:
URL: https://github.com/apache/openwhisk/pull/4954#discussion_r479960184
##########
File path: tests/src/test/scala/system/basic/WskSequenceTests.scala
##########
@@ -501,6 +501,46 @@ class WskSequenceTests extends TestHelpers with WskTestHelpers with StreamLoggin
checkEchoSeqRuleResult(newRun, seqName, JsObject(newPayload))
}
+ it should "run a sub-action even if it is updated while the sequence action is running" in withAssetCleaner(wskprops) {
Review comment:
I've added a test case. And this test currently does not pass on the master 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org