Skip to content

[improve][broker] Decouple ManagedLedger interfaces from the current implementation - #23311

Merged
lhotari merged 5 commits into
apache:masterfrom
lhotari:lh-managed-ledger-interface-improvements
Sep 17, 2024
Merged

[improve][broker] Decouple ManagedLedger interfaces from the current implementation#23311
lhotari merged 5 commits into
apache:masterfrom
lhotari:lh-managed-ledger-interface-improvements

Conversation

@lhotari

@lhotari lhotari commented Sep 16, 2024

Copy link
Copy Markdown
Member

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:

  1. To allow for alternative ManagedLedger implementations in Pulsar 4.0.
  2. 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

Since the ManagedLedger interface is not exposed externally and this PR doesn't introduce implementation changes, a formal PIP (Pulsar Improvement Proposal) is not required. The mailing list discussion and the review process for this PR are considered sufficient for proceeding with these changes.

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

  1. Decouple from ManagedLedgerImpl:

    • Add required methods from ManagedLedgerImpl to the ManagedLedger interface
    • Update relevant code to use the interface instead of the implementation
  2. Decouple from ManagedLedgerFactoryImpl:

    • Add necessary methods from ManagedLedgerFactoryImpl to the ManagedLedgerFactory interface
    • Refactor code to depend on the interface rather than the concrete implementation
  3. Decouple from ManagedCursorImpl:

    • Enhance the ManagedCursor interface with required methods from ManagedCursorImpl
    • Update code to use the interface methods instead of implementation-specific ones
  4. Introduce ReadOnlyManagedLedger interface:

    • Extract this interface to decouple from ReadOnlyManagedLedgerImpl
    • Adjust code to utilize the new interface where appropriate
  5. Decouple OpAddEntry from ManagedLedgerInterceptor:

    • add interface AddEntryOperation to be used by the beforeAddEntry method
    • revisit the method return type since it should be void
    • Adjust code to utilize the new interface where appropriate
    • Eliminate the use of AddEntryOperation in processPayloadBeforeLedgerWrite since mutation should happen with the PayloadProcessorHandle in that case.
  6. Decouple LedgerHandle from ManagedLedgerInterceptor:

    • add interface LastEntryHandle to be used by the onManagedLedgerLastLedgerInitialize method
    • Adjust code to utilize the new interface where appropriate
  7. Additional minor changes:

    • Adjust method signatures and parameter types as needed
    • Refactor any code that directly referenced implementation classes

Documentation

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

…implementation

Co-authored-by: Yan Zhao <horizonzy@apache.org>
Co-authored-by: Yong Zhang <zhangyong1025.zy@gmail.com>
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 49.29078% with 286 lines in your changes missing coverage. Please review.

Project coverage is 74.51%. Comparing base (bbc6224) to head (e376376).
Report is 580 commits behind head on master.

Files with missing lines Patch % Lines
...kkeeper/mledger/impl/ManagedLedgerFactoryImpl.java 10.33% 213 Missing and 4 partials ⚠️
...sar/broker/service/persistent/PersistentTopic.java 83.45% 16 Missing and 6 partials ⚠️
...ker/service/persistent/PersistentSubscription.java 50.00% 11 Missing and 2 partials ⚠️
...ice/persistent/PersistentMessageExpiryMonitor.java 50.00% 5 Missing and 5 partials ⚠️
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java 80.95% 5 Missing and 3 partials ⚠️
...che/pulsar/broker/service/BacklogQuotaManager.java 69.23% 4 Missing ⚠️
...eper/mledger/impl/ManagedLedgerOfflineBacklog.java 62.50% 2 Missing and 1 partial ⚠️
...r/impl/SnapshotSegmentAbortedTxnProcessorImpl.java 60.00% 2 Missing ⚠️
...nsaction/pendingack/impl/PendingAckHandleImpl.java 50.00% 1 Missing and 1 partial ⚠️
...g/apache/pulsar/compaction/CompactedTopicImpl.java 0.00% 2 Missing ⚠️
... and 3 more
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23311      +/-   ##
============================================
+ Coverage     73.57%   74.51%   +0.93%     
- Complexity    32624    34321    +1697     
============================================
  Files          1877     1929      +52     
  Lines        139502   145149    +5647     
  Branches      15299    15875     +576     
============================================
+ Hits         102638   108153    +5515     
+ Misses        28908    28723     -185     
- Partials       7956     8273     +317     
Flag Coverage Δ
inttests 27.63% <17.37%> (+3.04%) ⬆️
systests 24.67% <19.68%> (+0.35%) ⬆️
unittests 73.88% <49.11%> (+1.03%) ⬆️

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

Files with missing lines Coverage Δ
...a/org/apache/bookkeeper/mledger/ManagedCursor.java 31.57% <ø> (-3.72%) ⬇️
...a/org/apache/bookkeeper/mledger/PositionBound.java 100.00% <100.00%> (ø)
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java 80.23% <100.00%> (+0.93%) ⬆️
...g/apache/bookkeeper/mledger/impl/OpFindNewest.java 89.85% <100.00%> (+0.30%) ⬆️
...ava/org/apache/bookkeeper/mledger/impl/OpScan.java 82.53% <ø> (ø)
.../mledger/impl/OpenTelemetryManagedCursorStats.java 100.00% <100.00%> (ø)
.../mledger/impl/OpenTelemetryManagedLedgerStats.java 100.00% <100.00%> (ø)
...he/bookkeeper/mledger/impl/ReadOnlyCursorImpl.java 95.00% <ø> (ø)
...keeper/mledger/impl/ReadOnlyManagedLedgerImpl.java 56.75% <ø> (+3.13%) ⬆️
...n/java/org/apache/pulsar/broker/PulsarService.java 83.66% <100.00%> (+1.29%) ⬆️
... and 27 more

... and 542 files with indirect coverage changes

@lhotari
lhotari force-pushed the lh-managed-ledger-interface-improvements branch from e376376 to 212ccf8 Compare September 17, 2024 05:11
Comment thread managed-ledger/src/main/java/org/apache/bookkeeper/mledger/PositionBound.java Outdated
lhotari and others added 2 commits September 17, 2024 00:21
Co-authored-by: Yunze Xu <xyzinfernity@163.com>
Co-authored-by: Yunze Xu <xyzinfernity@163.com>
hanmz pushed a commit to hanmz/pulsar that referenced this pull request Feb 12, 2025
…implementation (apache#23311)

Co-authored-by: Yan Zhao <horizonzy@apache.org>
Co-authored-by: Yong Zhang <zhangyong1025.zy@gmail.com>
Co-authored-by: Yunze Xu <xyzinfernity@163.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs ready-to-test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants