Enable service discovery in controller too #8193

dongxiaoman · 2022-02-11T01:01:09Z

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:

Main idea is behind mkyong's article in detail in here
Added a property boolean named pinot.controller.service.auto.discovery to enable this behavior

Steps needed to make a class auto-created in PinotController:

Add hk2-metadata-generator module as a dependency in your JAR file
Tag your class with @org.jvnet.hk2.annotations.Service tag
Set pinot.controller.service.auto.discovery=true in your Controller's property config
Make sure your JAR file is loaded in class path(of coz)
(Optional) inject your class instance to other places

Upgrade Notes

Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)

Yes (Please label as 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-notes and complete the section on Release Notes)

Release Notes

New configuration options: pinot.controller.service.auto.discovery so Jersey services can be created automatically and then later injected to Endpoints.

Documentation

codecov-commenter · 2022-02-11T01:34:36Z

Codecov Report

Merging #8193 (9638065) into master (22b17c3) will decrease coverage by 1.19%.
The diff coverage is 33.33%.

@@             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

Flag	Coverage Δ
integration1	`?`
integration2	`27.55% <60.00%> (-0.03%)`	⬇️
unittests1	`67.90% <0.00%> (+0.01%)`	⬆️
unittests2	`14.27% <0.00%> (+0.06%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../controller/api/ControllerAdminApiApplication.java	`89.09% <0.00%> (-3.37%)`	⬇️
...ker/api/resources/BrokerEchoWithAutoDiscovery.java	`0.00% <0.00%> (ø)`
...api/resources/ControllerEchoWithAutoDiscovery.java	`0.00% <0.00%> (ø)`
...pache/pinot/core/api/AutoLoadedServiceForTest.java	`0.00% <0.00%> (ø)`
...va/org/apache/pinot/spi/utils/CommonConstants.java	`23.63% <ø> (ø)`
...pinot/broker/broker/BrokerAdminApiApplication.java	`93.02% <100.00%> (ø)`
...he/pinot/core/api/ServiceAutoDiscoveryFeature.java	`81.81% <100.00%> (ø)`
...pinot/minion/exception/TaskCancelledException.java	`0.00% <0.00%> (-100.00%)`	⬇️
...nverttorawindex/ConvertToRawIndexTaskExecutor.java	`0.00% <0.00%> (-100.00%)`	⬇️
...e/pinot/common/minion/MergeRollupTaskMetadata.java	`0.00% <0.00%> (-94.74%)`	⬇️
... and 106 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 22b17c3...9638065. Read the comment docs.

xiangfu0 · 2022-02-11T19:51:27Z

pinot-core/src/main/java/org/apache/pinot/core/api/ServiceAutoDiscoveryFeature.java

@@ -68,8 +69,8 @@
 * pinot-integration-tests/src/main/java/org/apache/pinot/broker/integration/tests/BrokerTestAutoLoadedService.java
 * </code>
 */
-public class BrokerServiceAutoDiscoveryFeature implements Feature {
-    private static final Logger LOGGER = LoggerFactory.getLogger(BrokerServiceAutoDiscoveryFeature.class);
+public class ServiceAutoDiscoveryFeature implements Feature {


What will happen if we have PinotServiceManager deploying controller/broker/server in the same JVM and all of them trying to load this ServiceAutoDiscoveryFeature?

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 · 2022-02-11T23:00:42Z

.../test/java/org/apache/pinot/integration/tests/ControllerServiceDiscoveryIntegrationTest.java

+      throws Exception {
+    String response = sendGetRequest(_controllerBaseApiUrl + "/test/echo/doge");
+    Assert.assertEquals(response, "doge");
+    response = sendGetRequest(_brokerBaseApiUrl + "/test/echo/doge");


@xiangfu0 the test here will fail if we do initialization only once

richardstartin · 2022-02-12T11:11:28Z

.../test/java/org/apache/pinot/integration/tests/ControllerServiceDiscoveryIntegrationTest.java

+    startBrokers(1);
+    startServers(1);
+
+  }


I’m surprised the linter passed, there should be a blank line here. We will need to check if checkstyle can catch this.

#8197

You triggered my OCD...

richardstartin · 2022-02-12T11:12:38Z

High level question: does this affect startup time?

xiangfu0 · 2022-02-13T10:38:48Z

High level question: does this affect startup time?

I think this more comes from each customized Feature implementation?

richardstartin · 2022-02-13T15:19:04Z

High level question: does this affect startup time?

I think this more comes from each customized Feature implementation?

Is there a discovery cost?

dongxiaoman · 2022-02-14T17:31:16Z

@richardstartin the discovery is only tried on the class written in META-INF/hk2-locator/default file. In my example the file contains [org.apache.pinot.broker.write.api.context.WriteApiContextProvider]S. The file is written automatically for the class tagged with @service tag.Based on code reading, it reads from that file and try to load it. There is no "scan blindly" in place, so it should be a fixed small cost.
Strictly speaking, this feature is not "AutoDiscovery", it is something "Loading-In-RunTime-with-Specified-Class-Name-in Resource"

richardstartin · 2022-02-14T18:07:02Z

Great, no concerns my end then.

dongxiaoman · 2022-02-15T17:03:39Z

@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

dongxiaoman changed the title ~~enable service discovery in controller too~~ Enable service discovery in controller too Feb 11, 2022

xiangfu0 requested review from Jackie-Jiang, xiangfu0 and apucher February 11, 2022 19:48

xiangfu0 reviewed Feb 11, 2022

View reviewed changes

dongxiaoman force-pushed the xd-controller-service-discovery branch from 7502ff1 to b28d6dd Compare February 11, 2022 22:43

dongxiaoman commented Feb 11, 2022

View reviewed changes

dongxiaoman force-pushed the xd-controller-service-discovery branch from 31f3b2e to 9638065 Compare February 11, 2022 23:02

richardstartin reviewed Feb 12, 2022

View reviewed changes

xiangfu0 approved these changes Feb 13, 2022

View reviewed changes

Xiaoman Dong added 3 commits February 14, 2022 09:27

enable service discovery in controller too

c084cc1

fix spotless check

6a12e1b

extra test to show that initialization are needed twice for two services

89ebdc0

dongxiaoman force-pushed the xd-controller-service-discovery branch from 9638065 to 89ebdc0 Compare February 14, 2022 17:31

fix checkstyle

bc12a3e

richardstartin merged commit 561621b into apache:master Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable service discovery in controller too #8193

Enable service discovery in controller too #8193

dongxiaoman commented Feb 11, 2022 •

edited

Loading

codecov-commenter commented Feb 11, 2022 •

edited

Loading

xiangfu0 Feb 11, 2022

dongxiaoman Feb 11, 2022

dongxiaoman Feb 11, 2022

richardstartin Feb 12, 2022

xiangfu0 Feb 13, 2022

richardstartin commented Feb 12, 2022

xiangfu0 commented Feb 13, 2022

richardstartin commented Feb 13, 2022

dongxiaoman commented Feb 14, 2022 •

edited

Loading

richardstartin commented Feb 14, 2022

dongxiaoman commented Feb 15, 2022

Enable service discovery in controller too #8193

Enable service discovery in controller too #8193

Conversation

dongxiaoman commented Feb 11, 2022 • edited Loading

Description

Background

Upgrade Notes

Release Notes

Documentation

codecov-commenter commented Feb 11, 2022 • edited Loading

Codecov Report

xiangfu0 Feb 11, 2022

Choose a reason for hiding this comment

dongxiaoman Feb 11, 2022

Choose a reason for hiding this comment

dongxiaoman Feb 11, 2022

Choose a reason for hiding this comment

richardstartin Feb 12, 2022

Choose a reason for hiding this comment

xiangfu0 Feb 13, 2022

Choose a reason for hiding this comment

richardstartin commented Feb 12, 2022

xiangfu0 commented Feb 13, 2022

richardstartin commented Feb 13, 2022

dongxiaoman commented Feb 14, 2022 • edited Loading

richardstartin commented Feb 14, 2022

dongxiaoman commented Feb 15, 2022

dongxiaoman commented Feb 11, 2022 •

edited

Loading

codecov-commenter commented Feb 11, 2022 •

edited

Loading

dongxiaoman commented Feb 14, 2022 •

edited

Loading