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.

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

[feat][broker][PIP-195] Implement delayed message bucket snapshot recover - part5 #18420

Merged
merged 3 commits into from
Dec 22, 2022

Conversation

coderzc
Copy link
Member

@coderzc coderzc commented Nov 11, 2022

Master Issue: #16763

Motivation

#16763

Modifications

Implement delayed message index bucket snapshot recover

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

Documentation

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

Matching PR in forked repository

PR in forked repository: coderzc#30

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 11, 2022
@coderzc coderzc self-assigned this Nov 20, 2022
@coderzc coderzc added this to the 2.12.0 milestone Nov 20, 2022
@coderzc coderzc added type/feature The PR added a new feature or issue requested a new feature area/broker labels Nov 21, 2022
Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

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

Looks generally good to me.
I just left some minor comments.

this.numberDelayedMessages = recoverBucketSnapshot();
}

private long recoverBucketSnapshot() throws RuntimeException {
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 better to add a method CompletableFuture<Void> initialize() to the DelayedDeliveryTracker to avoid blocking calls when creating the BucketDelayedDeliveryTracker instance

Copy link
Member Author

@coderzc coderzc Dec 19, 2022

Choose a reason for hiding this comment

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

It seems that we still need to block initialize method, when using DelayedDeliveryTrackerFactory to create DelayedEdeliverytracker

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, we can just keep it here for now.

int nextSegmentEntryId = nextSnapshotEntryIndex + 1;
loadMetaDataFuture.complete(nextSegmentEntryId);
}).exceptionally(ex -> {
loadMetaDataFuture.completeExceptionally(ex);
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we can simplify the logic to avoid creating the loadMetaDataFuture.
It will make the code easy to read.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems that he still needs loadMetaDataFuture, but I improve the code, make the code easy to read

@codelipenghui
Copy link
Contributor

/pulsarbot run-failure-checks

@codecov-commenter
Copy link

codecov-commenter commented Dec 19, 2022

Codecov Report

Merging #18420 (a7628e8) into master (27186a1) will increase coverage by 0.66%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18420      +/-   ##
============================================
+ Coverage     45.61%   46.28%   +0.66%     
+ Complexity    10728    10445     -283     
============================================
  Files           752      706      -46     
  Lines         72521    69078    -3443     
  Branches       7791     7398     -393     
============================================
- Hits          33083    31973    -1110     
+ Misses        35769    33509    -2260     
+ Partials       3669     3596      -73     
Flag Coverage Δ
unittests 46.28% <0.00%> (+0.66%) ⬆️

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

Impacted Files Coverage Δ
...rg/apache/pulsar/broker/delayed/bucket/Bucket.java 0.00% <0.00%> (ø)
...r/delayed/bucket/BucketDelayedDeliveryTracker.java 0.00% <0.00%> (ø)
.../pulsar/broker/delayed/bucket/ImmutableBucket.java 0.00% <0.00%> (ø)
.../transaction/buffer/metadata/AbortTxnMetadata.java 28.57% <0.00%> (-57.15%) ⬇️
...lsar/broker/loadbalance/impl/ThresholdShedder.java 3.27% <0.00%> (-30.06%) ⬇️
...oker/service/nonpersistent/NonPersistentTopic.java 42.02% <0.00%> (-17.50%) ⬇️
.../apache/pulsar/broker/admin/impl/PackagesBase.java 54.12% <0.00%> (-13.77%) ⬇️
...roker/service/persistent/MessageDeduplication.java 43.23% <0.00%> (-11.36%) ⬇️
.../org/apache/bookkeeper/mledger/impl/EntryImpl.java 69.62% <0.00%> (-11.27%) ⬇️
...er/impl/SingleSnapshotAbortedTxnProcessorImpl.java 70.65% <0.00%> (-10.31%) ⬇️
... and 197 more

Copy link
Member

@mattisonchao mattisonchao left a comment

Choose a reason for hiding this comment

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

LGTM

@codelipenghui codelipenghui merged commit 1666ecc into apache:master Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

6 participants