Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make activation polling for blocking invocations configurable #4088

Merged
merged 4 commits into from
Jan 9, 2019

Conversation

dubee
Copy link
Member

@dubee dubee commented Oct 29, 2018

Description

Allows controller activation polling for blocking invocations to be enabled or disable via configuration.

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.

@dubee dubee added the review Review for this PR has been requested and yet needs to be done. label Oct 29, 2018
@dubee dubee requested a review from csantanapr October 29, 2018 19:29
@@ -232,6 +232,8 @@

"CONFIG_whisk_transactions_header": "{{ transactions.header }}"

"CONFIG_whisk_controller_activation_polling": "{{ controller_activation_polling | default(true) }}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add _from_db perhaps?

@dubee dubee force-pushed the config-poll branch 6 times, most recently from a3a9cbc to a63f1ab Compare October 31, 2018 20:27
@codecov-io
Copy link

codecov-io commented Oct 31, 2018

Codecov Report

Merging #4088 into master will decrease coverage by 4.63%.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4088      +/-   ##
==========================================
- Coverage   85.86%   81.22%   -4.64%     
==========================================
  Files         148      148              
  Lines        7249     7255       +6     
  Branches      445      439       -6     
==========================================
- Hits         6224     5893     -331     
- Misses       1025     1362     +337
Impacted Files Coverage Δ
.../scala/src/main/scala/whisk/core/WhiskConfig.scala 94.26% <100%> (+0.09%) ⬆️
...isk/core/controller/actions/PrimitiveActions.scala 87.16% <83.33%> (-0.34%) ⬇️
...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%) ⬇️
...rc/main/scala/whisk/common/ForcibleSemaphore.scala 88.46% <0%> (-7.7%) ⬇️
...la/whisk/core/database/cosmosdb/CosmosDBUtil.scala 92% <0%> (-4%) ⬇️
...ain/scala/whisk/core/invoker/InvokerReactive.scala 81.98% <0%> (+17.11%) ⬆️

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 7668cc5...a63f1ab. Read the comment docs.

@dubee dubee added wip and removed review Review for this PR has been requested and yet needs to be done. labels Nov 14, 2018
@dubee
Copy link
Member Author

dubee commented Nov 14, 2018

Changed this back to a WIP state as the two active ack changes do not currently allow DB polling to be disabled.

@@ -593,11 +596,15 @@ protected[actions] trait PrimitiveActions {
// in case of an incomplete active-ack (record too large for example).
activeAckResponse.foreach {
case Right(activation) => result.trySuccess(Right(activation))
case _ => pollActivation(docid, context, result, i => 1.seconds + (2.seconds * i), maxRetries = 4)
case _ if (controllerActivationConfig.pollingFromDb) =>
pollActivation(docid, context, result, i => 1.seconds + (2.seconds * i), maxRetries = 4)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking at a case right now where if this pollActivation is removed, the acks are received in the wrong order in some cases (completion before result) - have you seen anything similar? unfortunately, it is something weird where it only happens on first run of the controller - subsequent runs of same or different actions don't exhibit the problem. Restarting controller, and the first invocation always gets the acks in wrong order again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I see the issue causing the out of order messages - the first call to AcknowledegmentMessage.parse(raw) is slow, resulting in out of order messages IFF the result + completion are sent at approx same time from invoker. I'll try to figure out a way to force the order of these.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tysonnorris is this related to #4134, which supposedly has been fixed by now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tysonnorris, I was seeing the same problem with large activations responses. Let me rebase these changes with #4134 to see if the problem has been resolved.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After rebasing it looks the two active ack changes no longer required DB polling.

@dubee dubee added review Review for this PR has been requested and yet needs to be done. and removed wip labels Nov 29, 2018
@dubee
Copy link
Member Author

dubee commented Nov 29, 2018

Looks like disabling DB polling is possible again after b803c64 and 7f571c3.

@rabbah
Copy link
Member

rabbah commented Jan 5, 2019

@dubee this lgtm - do you want to rebase to master?

@dubee
Copy link
Member Author

dubee commented Jan 8, 2019

@rabbah, rebased and ready to go.

@dubee dubee merged commit e510326 into apache:master Jan 9, 2019
BillZong pushed a commit to BillZong/openwhisk that referenced this pull request Nov 18, 2019
…#4088)

* Make activation polling for blocking invocations configurable

* Update Tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
controller enhancement review Review for this PR has been requested and yet needs to be done.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants