Skip to content

Conversation

@BewareMyPower
Copy link
Contributor

Documentation

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

Matching PR in forked repository

PR in forked repository:

…ely on BookKeeper when BookKeeper is not used
@BewareMyPower BewareMyPower self-assigned this May 13, 2025
@BewareMyPower BewareMyPower added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/broker labels May 13, 2025
@github-actions github-actions bot added PIP doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. labels May 13, 2025
Comment on lines +42 to +44
The following components leverages this factory to create their own `BookKeeper` instances:
- `BookkeeperSchemaStorageFactory`: The default schema registry storage configured by `schemaRegistryStorageClassName`
- `BookkeeperBucketSnapshotStorage`: A non-default built-in delayed delivery tracker configured by `delayedDeliveryTrackerFactoryClassName`
Copy link
Contributor

Choose a reason for hiding this comment

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

I’m not entirely sure why separate BookKeeper instances are used for these two cases. While this doesn’t directly affect the goal of the proposal, should we consider addressing or revisiting this as part of a broader cleanup?

Comment on lines +68 to +70
| Schema Registry | Fail fast |
| Delayed Delivery Tracker | Fail fast if it's `BucketDelayedDeliveryTrackerFactory` |
| Compaction Service | Fallback to a dummy implementation |
Copy link
Contributor

Choose a reason for hiding this comment

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

These three items refer to Pulsar’s default implementations, correct? If users have provided their own plugin implementations, they shouldn’t be affected — is that understanding accurate?

```java
public interface BookkeeperManagedLedgerStorageClass extends ManagedLedgerStorageClass {
/* ... */
BookKeeperClientFactory getBookKeeperClientFactory();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it designed to be used by BookkeeperSchemaStorageFactory and BookkeeperBucketSnapshotStorage?

I mean, BookkeeperManagedLedgerStorageClass already has getBookKeeperClient, and should all Pulsar's internal implementations use the same bookkeeper client? Maybe I missed something.

Co-authored-by: Penghui Li <[email protected]>
@codelipenghui codelipenghui added this to the 4.1.0 milestone Jun 20, 2025
@coderzc coderzc modified the milestones: 4.1.0, 4.2.0 Sep 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/broker doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. PIP type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants