[GitHub] [openwhisk] rabbah commented on a change in pull request #4582: Action container log collection does not wait for sentinel on developer error

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

rabbah commented on a change in pull request #4582: Action container log collection does not wait for sentinel on developer error
URL: https://github.com/apache/openwhisk/pull/4582#discussion_r313286977
 
 

 ##########
 File path: core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/docker/DockerContainer.scala
 ##########
 @@ -281,9 +309,11 @@ class DockerContainer(protected val id: ContainerId,
       }
       .via(new CompleteAfterOccurrences(_.containsSlice(DockerContainer.byteStringSentinel), 2, waitForSentinel))
       // As we're reading the logs after the activation has finished the invariant is that all loglines are already
-      // written and we mostly await them being flushed by the docker daemon. Therefore we can timeout based on the time
+      // written and we mostly await them being flushed by the docker daemon. Therefore we can time out based on the time
       // between two loglines appear without relying on the log frequency in the action itself.
       .idleTimeout(waitForLogs)
+      // Apply an overall time limit for this log collecting and processing stream.
+      .completionTimeout(logProcessingTimeout)
 
 Review comment:
   It looks to me like this is the crux of the patch - limit the log extraction until sentinel is found. I don’t know what a reasonable rate should be but up to 2s/1MB strikes me as long. I like Markus’ suggestion and wonder what the drain rate might be under load. That’s especially when waiting is on the critical path. 

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


With regards,
Apache Git Services