rabbah commented on a change in pull request #2658: Treat a timed out active
ack as failed activation in invokerhealth protocol
URL:
https://github.com/apache/incubator-openwhisk/pull/2658#discussion_r140473503
##########
File path:
core/controller/src/main/scala/whisk/core/loadBalancer/LoadBalancerService.scala
##########
@@ -125,18 +125,34 @@ class LoadBalancerService(config: WhiskConfig, instance:
InstanceId, entityStore
*/
private def processCompletion(response: Either[ActivationId,
WhiskActivation],
tid: TransactionId,
- forced: Boolean): Unit = {
+ forced: Boolean,
+ invoker: InstanceId): Unit = {
val aid = response.fold(l => l, r => r.activationId)
+
+ // treat left as success (as it is the result a the message exceeding the
bus limit)
+ // treat timed out active ack as failure to let the invoker become
unhealthy
+ val isSuccess = response.fold(l => true, r => !r.response.isWhiskError)
+
loadBalancerData.removeActivation(aid) match {
case Some(entry) =>
logging.info(this, s"${if (!forced) "received" else "forced"} active
ack for '$aid'")(tid)
+ // If we receive an active ack (forced = false) we send the
activationResult. If the timeout
+ // is executed earlier, we treat the activation as failed (forced =
true).
+ // At this point no health actions are handled, because they don't
have an entry in loadBalancerData.
+ invokerPool ! InvocationFinishedMessage(invoker, isSuccess && !forced)
if (!forced) {
entry.promise.trySuccess(response)
} else {
entry.promise.tryFailure(new Throwable("no active ack received"))
}
case None =>
// the entry was already removed
+ // This could happen if this is the active ack of an health action.
Another reason is
+ // that this method is called twice. On activeAcks and on timeouts.
The first hit removes the entry.
+ // We send the InvocationFinishedMessage only, when processCompletion
has not been called by the
+ // timeout (forced = false). Health actions don't have a timeout, so
there is no problem, if they
+ // are very long in the queue.
+ if (!forced) invokerPool ! InvocationFinishedMessage(invoker,
isSuccess)
Review comment:
we could probably distinguish better between activate acks matching None for
the two cases described:
if the `response` is left, _it should not be a health action_. So I wonder
if can add two case matches here:
```
case None if response.isLeft => // this is a clean up active ack, drop it
case /* None and response.isRight */ => // this is a health action
----------------------------------------------------------------
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:
[email protected]
With regards,
Apache Git Services
