[improve][broker][PIP-384] Decouple Bookkeeper client from ManagedLedgerStorage and enable multiple ManagedLedgerFactory instances by lhotari · Pull Request #23313 · apache/pulsar

lhotari · 2024-09-17T13:50:59Z

This PR is partial implementation for "PIP-384: ManagedLedger interface decoupling", #23363.

Motivation

This PR is part of an initiative to clean up and decouple the ManagedLedger interfaces from their current implementation in preparation for Pulsar 4.0. The primary goals are:

To allow for alternative ManagedLedger implementations in Pulsar 4.0.
To improve the overall architecture by reducing coupling between core Pulsar components and specific ManagedLedger implementations.

This work stems from a community discussion on the Apache Pulsar mailing list: Preparing for Pulsar 4.0: cleaning up the Managed Ledger interfaces

This PR adds supports for multiple ManagedLedger implementations when using a custom ManagedLedgerStorage implementation. The benefit of this is that a Bookkeeper implementation can co-exist with a custom ManagedLedger implementation in the same Pulsar cluster.

Scope

This PR aims to make minimal, focused changes to achieve the decoupling goal. It does not include:

Adding comprehensive JavaDocs to the interfaces (this will be addressed in future PRs)
Changing any external APIs or behaviors

Modifications

add managedLedgerStorageClassName to PersistencePolicies to control the ManagedLedgerFactory instance that is used for a particular namespace or topic.
add storageClassName to ManagedLedgerConfig, this field is read from PersistencePolicies (namespace or topic policies)
modify ManagedLedgerStorage interface to support getting a ManagedLedgerFactory with the storageClassName
change topics to use this for selecting the ManagedLedgerFactory instance
for existing code that is not aware of multiple choices, use the default storage class and the default ManagedLedgerFactory instance.

Documentation

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

codecov-commenter · 2024-10-08T01:32:04Z

Codecov Report

Attention: Patch coverage is 72.93578% with 59 lines in your changes missing coverage. Please review.

Project coverage is 74.52%. Comparing base (bbc6224) to head (3189e61).
Report is 640 commits behind head on master.

Files with missing lines	Patch %	Lines
...ion/pendingack/impl/MLPendingAckStoreProvider.java	66.07%	18 Missing and 1 partial ⚠️
...va/org/apache/pulsar/broker/service/ServerCnx.java	68.75%	8 Missing and 2 partials ⚠️
...pulsar/broker/admin/impl/PersistentTopicsBase.java	58.82%	7 Missing ⚠️
...sar/broker/service/persistent/PersistentTopic.java	56.25%	6 Missing and 1 partial ⚠️
...ache/pulsar/broker/ManagedLedgerClientFactory.java	60.00%	3 Missing and 1 partial ⚠️
...rg/apache/pulsar/broker/service/BrokerService.java	93.02%	2 Missing and 1 partial ⚠️
...r/stats/prometheus/PrometheusMetricsGenerator.java	70.00%	2 Missing and 1 partial ⚠️
...n/java/org/apache/pulsar/broker/PulsarService.java	81.81%	1 Missing and 1 partial ⚠️
...r/impl/SnapshotSegmentAbortedTxnProcessorImpl.java	66.66%	2 Missing ⚠️
...pache/pulsar/broker/admin/impl/NamespacesBase.java	0.00%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #23313      +/-   ##
============================================
+ Coverage     73.57%   74.52%   +0.94%     
- Complexity    32624    34581    +1957     
============================================
  Files          1877     1937      +60     
  Lines        139502   145539    +6037     
  Branches      15299    15901     +602     
============================================
+ Hits         102638   108461    +5823     
+ Misses        28908    28761     -147     
- Partials       7956     8317     +361

Flag	Coverage Δ
inttests	`27.77% <35.32%> (+3.19%)`	⬆️
systests	`24.53% <19.72%> (+0.21%)`	⬆️
unittests	`73.87% <72.93%> (+1.02%)`	⬆️

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

Files with missing lines	Coverage Δ
...apache/bookkeeper/mledger/ManagedLedgerConfig.java	`96.49% <100.00%> (+0.19%)`	⬆️
...pulsar/broker/TransactionMetadataStoreService.java	`68.77% <100.00%> (+0.26%)`	⬆️
...e/pulsar/broker/stats/metrics/AbstractMetrics.java	`54.05% <100.00%> (ø)`
...sar/broker/stats/metrics/ManagedLedgerMetrics.java	`100.00% <100.00%> (ø)`
...he/pulsar/broker/storage/ManagedLedgerStorage.java	`100.00% <100.00%> (ø)`
...ava/org/apache/pulsar/admin/cli/CmdNamespaces.java	`79.11% <ø> (+0.38%)`	⬆️
.../org/apache/pulsar/admin/cli/CmdTopicPolicies.java	`89.61% <ø> (ø)`
...in/java/org/apache/pulsar/admin/cli/CmdTopics.java	`79.98% <ø> (-1.20%)`	⬇️
...pache/pulsar/broker/admin/impl/NamespacesBase.java	`76.78% <0.00%> (+3.70%)`	⬆️
...lsar/common/policies/data/PersistencePolicies.java	`86.95% <85.71%> (+2.74%)`	⬆️
... and 9 more

... and 597 files with indirect coverage changes

BewareMyPower · 2024-10-09T12:51:30Z

+     * @return storage class instance, or empty if not found
     */
-    BookKeeper getBookKeeperClient();
+    Optional<ManagedLedgerStorageClass> getManagedLedgerStorageClass(String name);


This method is hard to understand. From the perspective of customizing a managed ledger storage, what I want to know is that when could this method be called and how could the name be.

It seems that the name comes from ManagedLedgerConfig#storageClassName. But how could this config be changed?

It seems that it can only be set via the topic policies.

This PR doesn't consider how the ManagedLedgerStorage implementation configures the default value. It's possible to override the storage class name at namespace or topic level in persistence policies.

BewareMyPower · 2024-10-09T12:54:17Z

-                v -> transactionMetadataStoreProvider.openStore(tcId, pulsarService.getManagedLedgerFactory(), v,
+                v -> transactionMetadataStoreProvider.openStore(tcId,
+                        pulsarService.getManagedLedgerStorage().getManagedLedgerStorageClass(v.getStorageClassName())
+                                .get().getManagedLedgerFactory(), v,


NPE will happen if getManagedLedgerStorageClass returns empty. This case is not handled well

That's correct. This could be improved.

…gerStorage and enable multiple ManagedLedgerFactory instances (apache#23313)

lhotari added this to the 4.0.0 milestone Sep 17, 2024

lhotari self-assigned this Sep 17, 2024

lhotari requested review from Technoboy-, codelipenghui, hangc0276, horizonzy, mattisonchao, merlimat and zymap September 17, 2024 13:51

lhotari added the ready-to-test label Sep 17, 2024

github-actions Bot added the doc-required Your PR changes impact docs and you will update later. label Sep 17, 2024

lhotari force-pushed the lh-managed-ledger-storage-decouple branch 2 times, most recently from 4dd0759 to 599224c Compare September 23, 2024 15:16

lhotari mentioned this pull request Sep 27, 2024

[improve] PIP-384: ManagedLedger interface decoupling #23363

Merged

4 tasks

merlimat approved these changes Oct 4, 2024

View reviewed changes

lhotari added 3 commits October 7, 2024 18:40

[improve][broker] Decouple Bookkeeper client from ManagedLedgerStorage

f88d407

Make it possible to spy ManagedLedgerStorage method calls in tests

537f2d5

Fix PendingAckPersistentTest.testGetManagedLegerConfigFailThenUnload

780f550

lhotari force-pushed the lh-managed-ledger-storage-decouple branch from 599224c to 780f550 Compare October 7, 2024 15:40

lhotari added 3 commits October 8, 2024 00:49

Fix ServerCnxTest

b922efa

Fix PersistentTopicTest

595a5e6

Fix PersistentDispatcherFailoverConsumerTest

3189e61

lhotari merged commit 731ec83 into apache:master Oct 8, 2024

BewareMyPower reviewed Oct 9, 2024

View reviewed changes

lhotari mentioned this pull request Oct 19, 2024

[fix][test] Prevent OOM in test by not spying invocations in SimpleProducerConsumerTest #23486

Merged

4 tasks

hanmz pushed a commit to hanmz/pulsar that referenced this pull request Feb 12, 2025

[improve][broker][PIP-384] Decouple Bookkeeper client from ManagedLed…

65ca9e9

…gerStorage and enable multiple ManagedLedgerFactory instances (apache#23313)

poorbarcode mentioned this pull request Dec 24, 2025

[fix][broker]Topic deleting failed after removed local cluster from namespace policies #25114

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[improve][broker][PIP-384] Decouple Bookkeeper client from ManagedLedgerStorage and enable multiple ManagedLedgerFactory instances - #23313

[improve][broker][PIP-384] Decouple Bookkeeper client from ManagedLedgerStorage and enable multiple ManagedLedgerFactory instances#23313
lhotari merged 6 commits into
apache:masterfrom
lhotari:lh-managed-ledger-storage-decouple

lhotari commented Sep 17, 2024 •

edited

Loading

Uh oh!

codecov-commenter commented Oct 8, 2024 •

edited

Loading

Uh oh!

BewareMyPower Oct 9, 2024

Uh oh!

BewareMyPower Oct 9, 2024

Uh oh!

lhotari Oct 9, 2024

Uh oh!

BewareMyPower Oct 9, 2024

Uh oh!

lhotari Oct 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

lhotari commented Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Scope

Modifications

Documentation

Uh oh!

codecov-commenter commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

BewareMyPower Oct 9, 2024

Choose a reason for hiding this comment

Uh oh!

BewareMyPower Oct 9, 2024

Choose a reason for hiding this comment

Uh oh!

lhotari Oct 9, 2024

Choose a reason for hiding this comment

Uh oh!

BewareMyPower Oct 9, 2024

Choose a reason for hiding this comment

Uh oh!

lhotari Oct 9, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lhotari commented Sep 17, 2024 •

edited

Loading

codecov-commenter commented Oct 8, 2024 •

edited

Loading