[improve][broker]Enable custom metadata stores

[andrasbeni]

Motivation

Pulsar can use different metadata stores (ZK, etcd, etc.). These metadata stores are built into Pulsar itself, and there has been no way of using a custom-made metadata store. This change allows the loading of metadata store implementations from the classpath by providing a system property.

Modifications

• Added MetadataStoreProvider interface
• MetadataStoreFactoryImpl checks System property pulsar.metadatastore.providers for provider class names
• If any of the loaded providers supports the URL scheme provided to MetadataStoreFactoryImpl, it uses the provider to create the MetadataStoreObject.
• Added unit test.

Verifying this change

• Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

• Added unit tests for MetadataStoreFactoryImpl

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

If the box was checked, please highlight the changes

• Dependencies (add or upgrade a dependency)
• The public API
• The schema
• The default values of configurations
• The threading model
• The binary protocol
• The REST endpoints
• The admin CLI options
• The metrics
• Anything that affects deployment

Documentation

• doc
• doc-required
• doc-not-needed
• doc-complete

Matching PR in forked repository

PR in forked repository: andrasbeni#10

[andrasbeni]

/pulsarbot rerun-failure-checks

[codecov-commenter]

Codecov Report

Merging #19208 (747b18b) into master (d7f8f56) will decrease coverage by 6.45%.
The diff coverage is 59.52%.

@@             Coverage Diff              @@
##             master   #19208      +/-   ##
============================================
- Coverage     47.89%   41.44%   -6.45%     
- Complexity    10871    18086    +7215     
============================================
  Files           713     1790    +1077     
  Lines         69730   134266   +64536     
  Branches       7496    14797    +7301     
============================================
+ Hits          33394    55647   +22253     
- Misses        32631    71879   +39248     
- Partials       3705     6740    +3035     

Flag	Coverage Δ
inttests	20.95% <59.52%> (?)
systests	24.93% <59.52%> (?)
unittests	50.17% <ø> (+2.28%)	⬆️

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

Impacted Files	Coverage Δ
...apache/pulsar/metadata/impl/EtcdMetadataStore.java	0.42% <33.33%> (ø)
...pulsar/metadata/impl/LocalMemoryMetadataStore.java	0.93% <33.33%> (ø)
...che/pulsar/metadata/impl/RocksdbMetadataStore.java	0.34% <33.33%> (ø)
...pulsar/metadata/impl/MetadataStoreFactoryImpl.java	55.00% <60.71%> (ø)
...g/apache/pulsar/metadata/impl/ZKMetadataStore.java	63.54% <100.00%> (ø)
...ava/org/apache/bookkeeper/mledger/util/Errors.java	33.33% <0.00%> (-66.67%)	⬇️
...kkeeper/mledger/impl/ManagedLedgerFactoryImpl.java	19.23% <0.00%> (-31.24%)	⬇️
...ar/broker/loadbalance/impl/BundleSplitterTask.java	54.00% <0.00%> (-28.00%)	⬇️
...g/apache/bookkeeper/mledger/ManagedLedgerInfo.java	0.00% <0.00%> (-22.23%)	⬇️
...a/org/apache/bookkeeper/mledger/ManagedCursor.java	30.76% <0.00%> (-15.39%)	⬇️
... and 1316 more

[andrasbeni]

/pulsarbot rerun-failure-checks

[asafm]

I didn't know java supports that (I'm used to it in Kotlin).

[nodece]

It looks unnecessary.

[andrasbeni]

It certainly does. But I think it is a good practice to leave the static state as it was before the test, regardless of the probability of it being non-null.
On the other hand, I think it's possible for someone using this very feature to run tests with their own provider configured in system properties.

[andrasbeni]

/pulsarbot rerun-failure-checks

[andrasbeni]

/pulsarbot rerun-failure-checks

[asafm]

Sorry about this. I think my comment thread has been hiding as resolved, so I'm resurfacing it if that's ok @andrasbeni

 String factoryClasses = System.getProperty(METADATASTORE_PROVIDERS_PROPERTY);

1. @andrasbeni The only tool I see here is CompactorTool, the rest are just plain Pulsar services that have access to Pulsar config. The Comparator tool itself has an argument @Parameter(names = {"-c", "--broker-conf"}, description = "Configuration file for Broker") so it also has access to the configuration file.
   So based on that, it seems that we can use the Pulsar configuration for those provider classes, no?

2. That one is a bit odd. If Pulsar is creating the BK client, why not provide the Client Metadata driver through the BK client itself? Why go through a System property to statically initialize? It may make sense inside BK itself. Maybe @hangc0276 knows. We can also call it when Pulsar initializes, as it has a method.

Based on (1) and (2), I don't understand why we can't use Pulsar configuration for that config @andrasbeni

Last question, @andrasbeni: does Pulsar have a mechanism that, based on an ENV variable or System property, sets a property in the configuration (like an override)? So you can define it in the pulsar configuration and use the technique you used to set it via a system property, making it less intrusive, less static code?