-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add SPI for invoker #4453
Add SPI for invoker #4453
Conversation
val producer = msgProvider.getProducer(config, Some(ActivationEntityLimit.MAX_ACTIVATION_LIMIT)) | ||
val invoker = try { | ||
new InvokerReactive(config, invokerInstance, producer, poolConfig) | ||
SpiLoader.get[InvokerProvider].instance(config, invokerInstance, producer, poolConfig, limitConfig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered many options to apply SPI in invoker.
For example, it would be possible to keep InvokerReactive
and apply SPI to ContainerPool
and ContainerProxy
level.
But I noticed that they are also highly dependent on InvokerReactive
.
So it would be more natural to apply SPI at the highest level of implementation.
It will abstract and hide the fundamental implementation such as ContainerPool
and ContainerProxy
from the top level Invoker implementation.
Also, there could be some dependent RestAPI implementations corresponding to the Invoker implementation.
So I added SPI provider for InvokerServer
which provides REST API as well.
} catch { | ||
case e: Exception => abort(s"Failed to initialize reactive invoker: ${e.getMessage}") | ||
} | ||
|
||
Scheduler.scheduleWaitAtMost(1.seconds)(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In some invoker implementations, health checking may not be performed via Kafka.
So I moved this to InvokerReactive
.
Codecov Report
@@ Coverage Diff @@
## master #4453 +/- ##
==========================================
- Coverage 85.33% 80.75% -4.58%
==========================================
Files 170 170
Lines 7929 7935 +6
Branches 552 550 -2
==========================================
- Hits 6766 6408 -358
- Misses 1163 1527 +364
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @style95
LGTM
PG1 - 4149. No issues. |
@dgrove-oss |
This is to allow multiple invoker implementations.
This is also a starting point to add new scheduler implementations described in https://cwiki.apache.org/confluence/display/OPENWHISK/New+architecture+proposal
Description
Related issue and scope
My changes affect the following components
Types of changes
Checklist: