Send active-ack after log collection for nonblocking activations.

[cbickel]

Until now, an active-ack is sent before logs of a container are collected. If one customer writes a lot of logs or if log-collection is slow for some other reason, the invoker already gets new activations, that are queueing up.
This PR changes the behavior, to send the active-ack (for non-blocking) activations after log collection is finished. For blocking actions, there are two active acks now. One with the response for the user and one to free up the space in the bookkeeping of the loadbalancer.

I've put this proposal on the dev-list for discussion: https://lists.apache.org/thread.html/726c802f38f3872d057f9adb6a52043eeb8fbd68b601c57bdb12d706@%3Cdev.openwhisk.apache.org%3E

I'll continue work on this PR, if we come to a conclusion how to proceed with this issue.

Related issue and scope

• I opened an issue to propose and discuss this change (#????)

My changes affect the following components

• API
• Controller
• Message Bus (e.g., Kafka)
• Loadbalancer
• Invoker
• Intrinsic actions (e.g., sequences, conductors)
• Data stores (e.g., CouchDB)
• Tests
• Deployment
• CLI
• General tooling
• Documentation

Types of changes

• Bug fix (generally a non-breaking change which closes an issue).
• Enhancement or new feature (adds new functionality).
• Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

• I signed an Apache CLA.
• I reviewed the style guides and followed the recommendations (Travis CI will check :).
• I added tests to cover my changes.
• My changes require further changes to the documentation.
• I updated the documentation where necessary.

[rabbah]

This is fine for non blocking. For blocking did you consider a second message (resource release in addition to active ack)?

…

-r

On Sep 25, 2018, at 4:29 AM, Christian Bickel ***@***.***> wrote: Until now, an active-ack is sent before logs of a container are collected. If one customer writes a lot of logs or if log-collection is slow for some other reason, the invoker already gets new activations, that are queueing up. This PR changes the behavior, to send the active-ack (for non-blocking) activations after log collection is finished. For blocking actions, there are two active acks now. One with the response for the user and one to free up the space in the bookkeeping of the loadbalancer. I've put this proposal on the dev-list for discussion: https://lists.apache.org/thread.html/726c802f38f3872d057f9adb6a52043eeb8fbd68b601c57bdb12d706@%3Cdev.openwhisk.apache.org%3E Related issue and scope I opened an issue to propose and discuss this change (#????) My changes affect the following components API Controller Message Bus (e.g., Kafka) Loadbalancer Invoker Intrinsic actions (e.g., sequences, conductors) Data stores (e.g., CouchDB) Tests Deployment CLI General tooling Documentation Types of changes Bug fix (generally a non-breaking change which closes an issue). Enhancement or new feature (adds new functionality). Breaking change (a bug fix or enhancement which changes existing behavior). Checklist: I signed an Apache CLA. I reviewed the style guides and followed the recommendations (Travis CI will check :). I added tests to cover my changes. My changes require further changes to the documentation. I updated the documentation where necessary. You can view, comment on, or merge this pull request online at: #4041 Commit Summary Send active-ack after log collection for nonblocking activations. File Changes M common/scala/src/main/scala/whisk/core/connector/Message.scala (5) M core/controller/src/main/scala/whisk/core/loadBalancer/ShardingContainerPoolBalancer.scala (83) M core/invoker/src/main/scala/whisk/core/containerpool/ContainerProxy.scala (22) M core/invoker/src/main/scala/whisk/core/invoker/InvokerReactive.scala (9) M tests/src/test/scala/whisk/core/containerpool/test/ContainerProxyTests.scala (2) Patch Links: https://github.com/apache/incubator-openwhisk/pull/4041.patch https://github.com/apache/incubator-openwhisk/pull/4041.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[cbickel]

@rabbah Using a new type of message, that is smaller, definitely makes sense.

[dubee]

Good find on this one, @cbickel!

It looks like isLogCollectionFinished is set to true for forced active acks. Can you describe the intended behavior for forced active acks with the new log flag?

[cbickel]

I'll change the code and reopen the PR afterwards.

[codecov-io]

Codecov Report

Merging #4041 into master will increase coverage by 4.42%.
The diff coverage is 80.35%.

@@            Coverage Diff             @@
##           master    #4041      +/-   ##
==========================================
+ Coverage   76.79%   81.22%   +4.42%     
==========================================
  Files         148      148              
  Lines        7219     7249      +30     
  Branches      438      445       +7     
==========================================
+ Hits         5544     5888     +344     
+ Misses       1675     1361     -314

Impacted Files	Coverage Δ
...cala/whisk/core/containerpool/ContainerProxy.scala	94.02% <100%> (+1.84%)	⬆️
...e/loadBalancer/ShardingContainerPoolBalancer.scala	84.07% <72%> (+54.6%)	⬆️
.../src/main/scala/whisk/core/connector/Message.scala	64.38% <78.57%> (+10.41%)	⬆️
...ain/scala/whisk/core/invoker/InvokerReactive.scala	81.98% <87.5%> (+81.98%)	⬆️
...core/database/cosmosdb/RxObservableImplicits.scala	0% <0%> (-100%)	⬇️
...core/database/cosmosdb/CosmosDBArtifactStore.scala	0% <0%> (-95.54%)	⬇️
...sk/core/database/cosmosdb/CosmosDBViewMapper.scala	0% <0%> (-92.6%)	⬇️
...whisk/core/database/cosmosdb/CosmosDBSupport.scala	0% <0%> (-83.34%)	⬇️
...abase/cosmosdb/CosmosDBArtifactStoreProvider.scala	0% <0%> (-58.83%)	⬇️
...la/whisk/core/database/cosmosdb/CosmosDBUtil.scala	92% <0%> (-4%)	⬇️
... and 45 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 538517b...bdcc503. Read the comment docs.

[cbickel]

PG3#2926 looks good

[vvraskin]

LGTM

[dgrove-oss]

I think this change is fine in and of itself, but it caused downstream problems for kube-deploy. apache/openwhisk-deploy-kube#328. Documenting here in case it impacts other downstream deployment projects like Mesos.