Skip to content

Conversation

@dongxiaoman
Copy link
Contributor

@dongxiaoman dongxiaoman commented Feb 1, 2022

Description

Added an optional "Auto Discovery" feature in PinotBroker so services tagged with @Service tag can be auto-loaded.

This will make Pinot Broker's Jersey behave very similar like Spring Framework in terms of service discovery.

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

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

  1. Add hk2-metadata-generator module as a dependency in your JAR file
  2. Tag your class with @org.jvnet.hk2.annotations.Service tag
  3. Set pinot.broker.service.auto.discovery=true in your Broker's property config
  4. Make sure your JAR file is loaded in class path(of coz)
  5. (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)
    No
    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)
    No
    Does this PR otherwise need attention when creating release notes? Things to consider:
  • New configuration options: pinot.broker.service.auto.discovery so Jersey services can be created automatically and then later injected to Endpoints.
  • Deprecation of configurations
  • Signature changes to public methods/interfaces
  • New plugins added or old plugins removed
  • Yes (Please label this PR as release-notes and complete the section on Release Notes)

Release Notes

Release note: Added Service Auto-discovery feature into PinotBroker so classes annotated with @Service and built with hk2-metadata-generator module can be automatically loaded.

Documentation

Will add later

* To make a class auto-loaded, what we need to do is to add the below dependency into project:
* <p>
* <pre>
* &lt;dependency&gt;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I use

{@code .... } 
, our check-style plugin will complain and fail the build.
Here I was forced to use HTML encode


@Service
@Singleton
public class BrokerTestAutoLoadedService {
Copy link
Contributor Author

@dongxiaoman dongxiaoman Feb 1, 2022

Choose a reason for hiding this comment

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

This class is not visible to BrokerAdminApiApplication.It is not registered by BrokerAdminApiApplication either.
But this service can be created and Injected into EchoWithAutoDiscovery endpoint above

</dependency>
<dependency>
<groupId>org.glassfish.hk2</groupId>
<artifactId>hk2-metadata-generator</artifactId>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This module is needed in any project that wish its classes to be auto-discovered.
When building the project, this module will create META-INF/hk2-locator/default file in JAR, and write the class name into that filer properly.
Later the ServiceAutoDiscoveryFeature class will automatically load the class.

@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2022

Codecov Report

Attention: Patch coverage is 64.70588% with 6 lines in your changes missing coverage. Please review.

Project coverage is 71.43%. Comparing base (3c98a44) to head (b81f7a5).
Report is 3749 commits behind head on master.

Files with missing lines Patch % Lines
...oker/broker/BrokerServiceAutoDiscoveryFeature.java 81.81% 2 Missing ⚠️
...ot/broker/api/resources/EchoWithAutoDiscovery.java 0.00% 2 Missing ⚠️
...integration/tests/BrokerTestAutoLoadedService.java 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #8107      +/-   ##
============================================
+ Coverage     71.35%   71.43%   +0.07%     
  Complexity     4303     4303              
============================================
  Files          1617     1620       +3     
  Lines         83874    83891      +17     
  Branches      12537    12538       +1     
============================================
+ Hits          59851    59928      +77     
+ Misses        19943    19880      -63     
- Partials       4080     4083       +3     
Flag Coverage Δ
integration1 29.03% <0.00%> (+0.03%) ⬆️
integration2 27.72% <84.61%> (+0.05%) ⬆️
unittests1 67.91% <ø> (+0.01%) ⬆️
unittests2 14.14% <0.00%> (-0.01%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@xiangfu0
Copy link
Contributor

xiangfu0 commented Feb 1, 2022

High level, this is good.
Question on other components, what if we want to apply same mechanism for controller/server/minion restful resources?
Shall we rename ServiceAutoDiscoveryFeature to BrokerServiceAutoDiscoveryFeature?

@dongxiaoman
Copy link
Contributor Author

High level, this is good. Question on other components, what if we want to apply same mechanism for controller/server/minion restful resources? Shall we rename ServiceAutoDiscoveryFeature to BrokerServiceAutoDiscoveryFeature?

Good question, I was thinking about enabling the same feature for controller/server/minion too, so did not call it BrokerServiceAutoDiscoveryFeature. Renamed to BrokerServiceAutoDiscoveryFeature for now. We can always refactor and enable later.

@dongxiaoman
Copy link
Contributor Author

@xiangfu0 the build has succeeded.

@xiangfu0 xiangfu0 merged commit 45c728b into apache:master Feb 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants