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

Introduce separate Akka dispatchers for CouchDB and Kafka Clients (#2954) #3515

Merged
merged 1 commit into from
Jun 13, 2018

Conversation

bwmcadams
Copy link
Contributor

@bwmcadams bwmcadams commented Apr 2, 2018

  • Rest of system continues to use the default system dispatcher

Refs #2954

Description

This reopens the previous #2956 PR; fork-join-executor wasn't the right choice previously and it is using Thread Pool dispatchers now, which are the correct way to handle this kind of IO separation with Akka. Defaults are set to what should be a reasonable starting point, especially given that couch and kafka now have their own separate execution pools from the "Default"/main system.

Kafka and CouchDB clients each get their own dispatchers

  • This should prevent all of the akka based stuff from competing constantly for threads
  • Kafka and Couch can be tuned now based on individual needs
  • Using thread-pool dispatchers, which are best suited for these processes (default is fork-join)
  • Dispatchers are defined in common reference.conf, so config can be overridden by dependents

Currently, OpenWhisk is sharing a single fork-join-executor across the board for the entire ActorSystem, which is less than ideal especially when dealing with potentially blocking IO.

Related issue and scope

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). (sort of a bug fix)
  • 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. (not easily testable in unit/integration)
  • My changes require further changes to the documentation.
  • I updated the documentation where necessary.

@markusthoemmes markusthoemmes requested a review from cbickel April 3, 2018 06:21
@rabbah rabbah added the review Review for this PR has been requested and yet needs to be done. label Jun 11, 2018
Copy link
Member

@rabbah rabbah left a comment

Choose a reason for hiding this comment

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

@cbickel this looks good to me - do you want to take a look otherwise we can merge this.

- Rest of system continues to use the default system dispatcher
- Kafka and CouchDB clients each get their own dispatchers
  * This should prevent all of the akka based stuff from competing constantly for threads
  * Kafka and Couch can be tuned now based on individual needs
  * Using thread-pool dispatchers, which are best suited for these processes (default is fork-join)
- Dispatchers are defined in common `reference.conf`, so config can be overridden by dependents

Refs apache#2954
@rabbah rabbah force-pushed the wip-bwm-dispatcher-cleanup branch from 3f4d46f to 74c4ac8 Compare June 11, 2018 04:25
@codecov-io
Copy link

codecov-io commented Jun 11, 2018

Codecov Report

Merging #3515 into master will increase coverage by <.01%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3515      +/-   ##
=========================================
+ Coverage      75%     75%   +<.01%     
=========================================
  Files         128     128              
  Lines        6081    6082       +1     
  Branches      398     389       -9     
=========================================
+ Hits         4561    4562       +1     
  Misses       1520    1520
Impacted Files Coverage Δ
.../whisk/core/database/RemoteCacheInvalidation.scala 0% <0%> (ø) ⬆️
...n/scala/whisk/core/database/CouchDbRestStore.scala 74.03% <100%> (ø) ⬆️
...n/scala/whisk/core/connector/MessageConsumer.scala 83.56% <100%> (ø) ⬆️
.../scala/whisk/core/database/CouchDbRestClient.scala 87.23% <100%> (+0.27%) ⬆️

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 7bd6474...74c4ac8. Read the comment docs.

@rabbah
Copy link
Member

rabbah commented Jun 11, 2018

@bwmcadams @chetanmeh this might be of use for other datastores so I wonder if we should rename the couch configuration to data store.

@chetanmeh
Copy link
Member

this might be of use for other datastores so I wonder if we should rename the couch configuration to data store.

+1

@bwmcadams
Copy link
Contributor Author

bwmcadams commented Jun 12, 2018 via email

@rabbah rabbah merged commit cd7196a into apache:master Jun 13, 2018
@chetanmeh
Copy link
Member

@bwmcadams #3779 introduces a new S3AttachmentStore where I was thinking to use the dedicated dispatcher. Do you plan to rename the dispatcher from couch-dispatcher to db-dispatcher?

@bwmcadams
Copy link
Contributor Author

@chetanmeh good question. I think the quandary is whether there's a single shared db-dispatcher or a specific one per database implementation. My preference/instinct would be to have one per database implementation. The way a particular database driver is implemented can vary vastly from one to the other in terms of what the best dispatcher tuning is for it. Sharing a single dispatcher may not be ideal.

@chetanmeh
Copy link
Member

@bwmcadams There are 2 aspects

  1. Have a dedicated separate dispatcher for db operations
  2. Dispatcher being tuned per DB driver implementation

Currently we have few storage impls which use Akka HTTP stack like CouchDB, S3 and ElasticSearch. So for them I think it would be better to have a generic db-dispatcher which is used by default. If any setup needs specific tuning then config can still be overridden for that deployment and later we can see if it makes sense to use a dedicated dispatcher for that kind of db.

So to start with lets have a generic db-dispatcher and later based on learning we can define specific one. This would allow us to at least meet 1 and still enabling option for 2.

Thoughts?

BillZong pushed a commit to BillZong/openwhisk that referenced this pull request Nov 18, 2019
…ache#3515)

- Rest of system continues to use the default system dispatcher
- Kafka and CouchDB clients each get their own dispatchers
  * This should prevent all of the akka based stuff from competing constantly for threads
  * Kafka and Couch can be tuned now based on individual needs
  * Using thread-pool dispatchers, which are best suited for these processes (default is fork-join)
- Dispatchers are defined in common `reference.conf`, so config can be overridden by dependents

Refs apache#2954
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants