Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[feat][broker][PIP-195] New bucket based delayed message tracker - interface&config&proto -part 1 #17344

Merged
merged 8 commits into from
Sep 14, 2022
Merged

[feat][broker][PIP-195] New bucket based delayed message tracker - interface&config&proto -part 1 #17344

merged 8 commits into from
Sep 14, 2022

Conversation

coderzc
Copy link
Member

@coderzc coderzc commented Aug 30, 2022 •
edited
Loading

Master Issue: #16763

Modifications

  • Add config of bucket based delayed message tracker
  • Add DelayedMessageIndexBucketSnapshotFormat.proto
  • Add BucketSnapshotStorage interface
  • Change DelayedDeliveryTracker interface

Documentation

Check the box below or label this PR directly.

Need to update docs?

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@coderzc coderzc changed the title [feat][broker][PIP-195] New bucket based delayed message tracker - part 1 [feat][broker][PIP-195] New bucket based delayed message tracker - interface&config&proto -part 1 Aug 30, 2022
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Aug 30, 2022
@coderzc
Copy link
Member Author

coderzc commented Sep 6, 2022

@codelipenghui @Technoboy- @gaoran10 PTAL

@codelipenghui codelipenghui added this to the 2.12.0 milestone Sep 6, 2022
@codelipenghui codelipenghui added type/feature The PR added a new feature or issue requested a new feature area/broker labels Sep 6, 2022
@coderzc coderzc force-pushed the bucket_delayed_interface branch from bd39d4a to 7a60585 Compare September 7, 2022 13:49
@coderzc coderzc force-pushed the bucket_delayed_interface branch from 5f229bc to 6951f7a Compare September 8, 2022 06:02
@liangyepianzhou liangyepianzhou self-requested a review September 9, 2022 10:25
@liangyepianzhou
Copy link
Contributor

Strange, The proposal has two other configurations delayedDeliveryTrackerFactoryClassName
delayedDeliverySharedIndexEnabled, which is not here.

@coderzc
Copy link
Member Author

coderzc commented Sep 9, 2022 •
edited
Loading

Strange, The proposal has two other configurations delayedDeliveryTrackerFactoryClassName
delayedDeliverySharedIndexEnabled, which is not here.

The delayedDeliveryTrackerFactoryClassName already exist. see: https://github.com/apache/pulsar/blob/master/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java#L333-L336
The delayedDeliverySharedIndexEnabled will be alone added when push "shared delayed message Index" PR.

@liangyepianzhou
Copy link
Contributor

liangyepianzhou commented Sep 9, 2022 •
edited
Loading

The delayedDeliveryTrackerFactoryClassName already exist. see: https://github.com/apache/pulsar/blob/master/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java#L333-L336

But you modified its comments, didn't you?
In my opinion, since this is a PR about this PIP-195 configuration, then we can put all the configurations in this PR. This is mainly to facilitate the review by the documentation team.

@coderzc
Copy link
Member Author

coderzc commented Sep 9, 2022 •
edited
Loading

But you modified its comments, didn't you? In my opinion, since this is a PR about this PIP-195 configuration, then we can put all the configurations in this PR. This is mainly to facilitate the review by the documentation team.

I am worried that someone will open which feature that has not been prepared, causing some problems.
If worry is not necessary, I can add them in this PR too.

@liangyepianzhou
Copy link
Contributor

liangyepianzhou commented Sep 9, 2022 •
edited
Loading

I am worried that someone will open which feature that has not been prepared, causing some problems.
If worry is not necessary, I can add them in this PR too.

What will be the impact?
Just a configuration item, nothing substantial, right?

@coderzc
Copy link
Member Author

coderzc commented Sep 9, 2022

I am worried that someone will open which feature that has not been prepared, causing some problems.
If worry is not necessary, I can add them in this PR too.

What will be the impact? Just a configuration item, nothing substantial, right?

Ok, I will add all the configurations.

gaoran10
gaoran10 approved these changes Sep 13, 2022
View reviewed changes
Copy link
Contributor

@gaoran10 gaoran10 left a comment

Choose a reason for hiding this comment

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

LGTM

@coderzc coderzc mentioned this pull request Sep 13, 2022
Merged
4 tasks
codelipenghui
codelipenghui approved these changes Sep 14, 2022
View reviewed changes
pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/BucketSnapshotStorage.java
Comment on lines +50 to +51
* @param firstSegmentEntryId entryId of first segment of sequence
* @param lastSegmentEntryId entryId of last segment of sequence
Copy link
Contributor

Choose a reason for hiding this comment

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

inclusive or exclusive?

Copy link
Member Author

Choose a reason for hiding this comment

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

inclusive

@codelipenghui codelipenghui merged commit 6bc3016 into apache:master Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gaoran10 gaoran10 gaoran10 approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@liangyepianzhou liangyepianzhou Awaiting requested review from liangyepianzhou

@congbobo184 congbobo184 Awaiting requested review from congbobo184

@momo-jun momo-jun Awaiting requested review from momo-jun

Assignees

@coderzc coderzc

Labels
area/broker doc-not-needed Your PR changes do not impact docs type/feature The PR added a new feature or issue requested a new feature
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@coderzc @liangyepianzhou @codelipenghui @gaoran10