Enable service discovery in controller too#8193
Enable service discovery in controller too#8193richardstartin merged 4 commits intoapache:masterfrom
Conversation
dongxiaoman
changed the title
Codecov Report
@@ Coverage Diff @@
## master #8193 +/- ##
============================================
- Coverage 71.36% 70.16% -1.20%
- Complexity 4308 4311 +3
============================================
Files 1623 1624 +1
Lines 84365 84369 +4
Branches 12657 12658 +1
============================================
- Hits 60208 59199 -1009
- Misses 20032 21077 +1045
+ Partials 4125 4093 -32
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
What will happen if we have PinotServiceManager deploying controller/broker/server in the same JVM and all of them trying to load this ServiceAutoDiscoveryFeature?
There was a problem hiding this comment.
A very good point. I added some code (static boolean) to make sure discovery is done once per JVM, and did the integration test that starts Broker+Controller in the same test.
The result shows that only one of the Broker/Controller can find service properly.
It seems that based on Jersey, Services/Features are isolated per Application (tied to ResourceConfig) level, so if there are two applications, the discovery has to be done twice.
dongxiaoman
force-pushed
the
xd-controller-service-discovery
branch
from
7502ff1 to
b28d6dd
Compare
There was a problem hiding this comment.
@xiangfu0 the test here will fail if we do initialization only once
dongxiaoman
force-pushed
the
xd-controller-service-discovery
branch
from
31f3b2e to
9638065
Compare
There was a problem hiding this comment.
I’m surprised the linter passed, there should be a blank line here. We will need to check if checkstyle can catch this.
There was a problem hiding this comment.
You triggered my OCD...
|
High level question: does this affect startup time? |
I think this more comes from each customized Feature implementation? |
Is there a discovery cost? |
|
dongxiaoman
force-pushed
the
xd-controller-service-discovery
branch
from
9638065 to
89ebdc0
Compare
|
Great, no concerns my end then. |
|
@xiangfu0 Gentle ping: build and test passed |
* enable service discovery in controller too * fix spotless check * extra test to show that initialization are needed twice for two services * fix checkstyle
4 participants
Description
Enable controller service auto-discovery in Jersey framework.
Also refactored the Broker's ServiceAutoDiscoveryFeature into a shared class for Pinot
Background
This is following #8107 to enable the service auto-creation in controller too.
It is found out that we may need to hook in new services in Pinot Controllers too.
The context can be found in previous PR, I will copy/paste the same text here for reference:
pinot.controller.service.auto.discoveryto enable this behaviorSteps needed to make a class auto-created in PinotController:
hk2-metadata-generatormodule as a dependency in your JAR file@org.jvnet.hk2.annotations.Servicetagpinot.controller.service.auto.discovery=truein your Controller's property configUpgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
backward-incompat, and complete the section below on Release Notes)Does this PR fix a zero-downtime upgrade introduced earlier?
Yes (Please label this as
backward-incompat, and complete the section below on Release Notes)Yes (Please label this PR as
release-notesand complete the section on Release Notes)Release Notes
pinot.controller.service.auto.discoveryso Jersey services can be created automatically and then later injected to Endpoints.Documentation