PIP-144: Making SchemaRegistry implementation configurable

[aparajita89]

Fixes #14101

Motivation

SchemaRegistryService is hardcoded to use SchemaRegistryServiceImpl in org.apache.pulsar.broker.service.schema.SchemaRegistryService#create

Modifications

broker.conf has a new config called schemaRegistryClassName which will have the name of the class to be used. the class is instantiated in SchemaRegistryService interface's static create method via reflection.

Verifying this change

• [ ] Make sure that the change passes the CI checks.

This change is already covered by existing tests which create a producer or consumer successfully.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

• Dependencies (does it add or upgrade a dependency): no
• The public API: no
• The schema: yes
• The default values of configurations: yes
• The wire protocol: no
• The rest endpoints: no
• The admin cli options: no
• Anything that affects deployment: no

Documentation

Check the box below or label this PR directly (if you have committer privilege).

Need to update docs?

• [x] doc-required this PR adds a new config which needs to be explained in the docs

[aparajita89]

comments from yesterday's review by @merlimat and ankur:

• schema storage might not be used in custom schema registry, it need not be passed in the constructor
• custom schema registry will require additional configs which will be present in ServiceConfiguration but it's not being passed to the constructor right now
• evaluate the existing approach once more keeping in mind the above points

[Huanli-Meng]

@aparajita89 do we also need to update the broker.config file? Thanks @Anonymitaet could you or Jun help follow up adding / updating docs about the PR? Thanks.

[aparajita89]

@Huanli-Meng this is an optional config. if no value is provided in the broker.conf file then the existing flow would go through. imo, we can add this config to the documentation but there isn't a need to update the config files unless someone really requires it.

[aparajita89]

@Anonymitaet could you let me know what changes i need to make for the documentation?

[Anonymitaet]

@aparajita89 how about adding the parameter and its description here https://pulsar.apache.org/docs/en/next/reference-configuration/#standalone?

[aparajita89]

@Anonymitaet thanks, i have added it in reference-configuration.md

[aparajita89]

Hello @aparajita89

Thanks for your initiative. Can you please start a PIP and a discussion on [email protected] for this feature?

sure, will do that

[aparajita89]

@eolivelli the PIP has been created, please take a look: https://github.com/apache/pulsar/issues/14395

[aparajita89]

@merlimat i've responded to your comments, could you take a look?

[codelipenghui]

@aparajita89 Would you please help to resolve the conflicts?

[aparajita89]

@codelipenghui yes, checking

[aparajita89]

@codelipenghui it's updated now

[aparajita89]

@codelipenghui could you update?

[codelipenghui]

@aparajita89

Please help check the failed check style:

Error:  src/main/java/org/apache/pulsar/broker/PulsarService.java:[1282] (sizes) LineLength: Line is longer than 120 characters (found 123).
Error:  src/main/java/org/apache/pulsar/broker/service/schema/validator/SchemaRegistryServiceWithSchemaDataValidator.java:[24,1] (imports) ImportOrder: Extra separation in import group before 'org.apache.pulsar.broker.PulsarServerException'
Error:  src/main/java/org/apache/pulsar/broker/service/schema/DefaultSchemaRegistryService.java:[26,1] (imports) ImportOrder: Extra separation in import group before 'org.apache.pulsar.broker.PulsarServerException'
Error:  Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.1.2:check (check-style) on project pulsar-broker: You have 3 Checkstyle violations. -> [Help 1]

[anvinjain]

@eolivelli Are you suggesting this from a runtime class conflict perspective? Schema storage in Pulsar is already pluggable (aside from the registry itself becoming pluggable in this PR) through this flag: schemaRegistryStorageClassName. Users are probably already making use of this and one way to avoid conflicts introduced by third party libs is that they end up using fat/shaded jars.

[eolivelli]

@eolivelli Are you suggesting this from a runtime class conflict perspective? Schema storage in Pulsar is already pluggable (aside from the registry itself becoming pluggable in this PR) through this flag: schemaRegistryStorageClassName. Users are probably already making use of this and one way to avoid conflicts introduced by third party libs is that they end up using fat/shaded jars.

Exactly. It is quite hard to deal with these problems, "fat/shaded jars" are a good trick, but I wonder if it is possible to address this problem the same way we are doing for many other components (Protocol Handlers, Additional Servlets, Package Management). Probably schemaRegistryStorageClassName was implemented when Pulsar didn't think much about extensibility problems.

[aparajita89]

@eolivelli looking into it.. @codelipenghui i have pushed the checkstyle fixes, looking into the remaining comments..

[aparajita89]

@eolivelli the org.apache.pulsar.common.nar.NarClassLoader supports loading of jar files as well and so, changing to NarClassLoader would not be a change in interface for the users. since this is not user facing, does it make sense to track this change in a separate github issue?

[aparajita89]

@eolivelli could you take a look?

[merlimat]

@eolivelli I think we should consider the NAR approach in a separate PR

[aparajita89]

@eolivelli i have added the integration test

[codelipenghui]

@eolivelli Please help review this PR again.

[codelipenghui]

ping @eolivelli Please help review this PR.

[github-actions[bot]]

The pr had no activity for 30 days, mark with Stale label.