[feat][broker][PIP-195] Implement BookkeeperBucketSnapshotStorage - part3 by coderzc · Pull Request #17677 · apache/pulsar

coderzc · 2022-09-15T16:26:42Z

Master Issue: #16763

Motivation

#16763

Modifications

Implement BookkeeperBucketSnapshotStorage and add unit test.

Verifying this change

Make sure that the change passes the CI checks.

Documentation

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)

Matching PR in forked repository
PR in forked repository: coderzc#5

github-actions · 2022-10-24T02:19:41Z

The pr had no activity for 30 days, mark with Stale label.

poorbarcode

Haven't finished the review yet, left some comments

poorbarcode

By the way, I noticed that the method below is only used by BucketDelayedDeliveryTracker. Should it be deleted from the interface DelayedDeliveryTracker

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/DelayedDeliveryTracker.java

Line 73 in 8246e3b

boolean containsMessage(long ledgerId, long entryId);

mattisonchao · 2022-11-18T03:21:12Z

+    @Override
+    public CompletableFuture<Long> createBucketSnapshot(SnapshotMetadata snapshotMetadata,
+                                                        List<SnapshotSegment> bucketSnapshotSegments) {
+        return createLedger()


I'm not sure if it would cause some problems when we created a lot of ledgers, but I think it would be OK for this 0-1 implementation.

@codelipenghui Could you please help confirm it?

mattisonchao · 2022-11-18T03:32:32Z

+                    } else {
+                        future.complete(entries);
+                    }
+                    closeLedger(handle);


We ignored the result or exception.

This is a resource release, I think it can not block the process

I still have concerns here, we call this method getLedgerEntryThenCloseLedger, but we don't ensure the ledger is closed after this method is returned. (because of the close behaviour running in the async background )

zymap · 2022-11-18T04:00:47Z

+                    } else {
+                        future.complete(handle);
+                    }
+                }, null, null);


Please add ledger metadata so that we can know which component is using this ledger through bookkeeper shell tools.
We have different components using ledgers, we need to add metadata on the ledger to know who is using it. You can check LedgerMetadataUtils to learn about how to add it.

I add metadata using bucketKey

coderzc · 2022-11-18T10:46:50Z

@poorbarcode @zymap @mattisonchao Please look at it again.

…ucket/BookkeeperBucketSnapshotStorage.java

codecov-commenter · 2022-11-29T04:57:49Z

Codecov Report

Merging #17677 (8a5c875) into master (a2c1534) will increase coverage by 0.10%.
The diff coverage is 6.40%.

❗ Current head 8a5c875 differs from pull request most recent head 64b7310. Consider uploading reports for the commit 64b7310 to get more accurate results

@@             Coverage Diff              @@
##             master   #17677      +/-   ##
============================================
+ Coverage     47.23%   47.34%   +0.10%     
+ Complexity    10430     9417    -1013     
============================================
  Files           692      626      -66     
  Lines         67766    59218    -8548     
  Branches       7260     6152    -1108     
============================================
- Hits          32009    28035    -3974     
+ Misses        32192    28140    -4052     
+ Partials       3565     3043     -522

Flag	Coverage Δ
unittests	`47.34% <6.40%> (+0.10%)`	⬆️

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

Impacted Files	Coverage Δ
...elayed/bucket/BookkeeperBucketSnapshotStorage.java	`0.00% <0.00%> (ø)`
...rg/apache/pulsar/broker/delayed/bucket/Bucket.java	`0.00% <0.00%> (ø)`
...yed/bucket/BucketSnapshotPersistenceException.java	`0.00% <0.00%> (ø)`
...d/bucket/BucketSnapshotSerializationException.java	`0.00% <0.00%> (ø)`
.../pulsar/broker/service/BrokerServiceException.java	`47.27% <0.00%> (+4.68%)`	⬆️
...ava/org/apache/pulsar/broker/service/Consumer.java	`69.75% <100.00%> (-2.42%)`	⬇️
.../transaction/buffer/metadata/AbortTxnMetadata.java	`28.57% <0.00%> (-57.15%)`	⬇️
...java/org/apache/pulsar/proxy/stats/TopicStats.java	`58.82% <0.00%> (-41.18%)`	⬇️
...ar/broker/loadbalance/impl/BundleSplitterTask.java	`60.00% <0.00%> (-20.00%)`	⬇️
...oker/service/nonpersistent/NonPersistentTopic.java	`42.02% <0.00%> (-15.61%)`	⬇️
... and 196 more

codelipenghui

Looks good to me. Just left a minor comment.

codelipenghui · 2022-12-19T04:13:16Z

+                                                        List<SnapshotSegment> bucketSnapshotSegments,
+                                                        String bucketKey) {
+        return createLedger(bucketKey)
+                .thenCompose(ledgerHandle -> addEntry(ledgerHandle, snapshotMetadata.toByteArray())


If the entry add failed, do we need to clean up the created Ledger?

I add clean up logical

zymap

LGTM

mattisonchao · 2022-12-19T08:39:55Z

+                                                        List<SnapshotSegment> bucketSnapshotSegments) {
+        List<CompletableFuture<Void>> addFutures = new ArrayList<>();
+        for (SnapshotSegment bucketSnapshotSegment : bucketSnapshotSegments) {
+            addFutures.add(addEntry(ledgerHandle, bucketSnapshotSegment.toByteArray()));


is it matter if we add 1,2 successful but 3,4 failed?

FutureUtil.waitForAll will make sure they will not be partially successful?

Yes, but some entries have already been added to this ledger. So we will throw this book away, right?

codelipenghui · 2022-12-19T09:00:14Z

                        .thenCompose(__ -> addSnapshotSegments(ledgerHandle, bucketSnapshotSegments))
                        .thenCompose(__ -> closeLedger(ledgerHandle))
-                        .thenApply(__ -> ledgerHandle.getId()));
+                        .thenApply(__ -> ledgerHandle.getId()).whenComplete((ledgerId, ex) -> {


If we add the exception handing to the end of the chain. Do we need to check the exception types? If it just failed to close the Ledger, we don't need to remove the ledger or just the Ledger creation failed.

Or we can just add exception handling for addEntry() method?

mattisonchao

Overall LGTM.

github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Sep 15, 2022

coderzc mentioned this pull request Sep 21, 2022

[feat][broker][PIP-195] Implement BucketDelayedDeliveryTrackerFactory and load BucketDelayedDeliveryTracker - part6 #17756

Merged

5 tasks

coderzc force-pushed the bucket_delayed_store branch from 37de8d6 to 5c61216 Compare September 21, 2022 08:43

github-actions Bot added the Stale label Oct 24, 2022

coderzc force-pushed the bucket_delayed_store branch from 9a23f4f to 4330cae Compare November 7, 2022 03:53

Implement BookkeeperBucketSnapshotStorage

1c5e771

coderzc force-pushed the bucket_delayed_store branch from 4330cae to 1c5e771 Compare November 7, 2022 03:56

poorbarcode reviewed Nov 11, 2022

View reviewed changes

Comment thread ...r/src/main/java/org/apache/pulsar/broker/delayed/bucket/BookkeeperBucketSnapshotStorage.java Outdated

improve code

475d108

poorbarcode approved these changes Nov 14, 2022

View reviewed changes

Comment thread ...r/src/main/java/org/apache/pulsar/broker/delayed/bucket/BookkeeperBucketSnapshotStorage.java

mattisonchao reviewed Nov 18, 2022

View reviewed changes

zymap reviewed Nov 18, 2022

View reviewed changes

coderzc force-pushed the bucket_delayed_store branch from 1f4965b to 068b37e Compare November 18, 2022 10:42

Address comment

3c17124

coderzc force-pushed the bucket_delayed_store branch from 068b37e to 3c17124 Compare November 18, 2022 10:50

github-actions Bot removed the Stale label Nov 19, 2022

coderzc self-assigned this Nov 20, 2022

coderzc added this to the 2.12.0 milestone Nov 20, 2022

zymap reviewed Nov 21, 2022

View reviewed changes

Comment thread ...r/src/main/java/org/apache/pulsar/broker/delayed/bucket/BookkeeperBucketSnapshotStorage.java Outdated

coderzc commented Nov 21, 2022

View reviewed changes

Comment thread ...r/src/main/java/org/apache/pulsar/broker/delayed/bucket/BookkeeperBucketSnapshotStorage.java Outdated

Comment thread ...r/src/main/java/org/apache/pulsar/broker/delayed/bucket/BookkeeperBucketSnapshotStorage.java Outdated

coderzc added 2 commits November 21, 2022 13:35

Update pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/b…

33dfa1a

…ucket/BookkeeperBucketSnapshotStorage.java

Update pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/b…

4a95e68

…ucket/BookkeeperBucketSnapshotStorage.java

coderzc added type/feature The PR added a new feature or issue requested a new feature area/broker ready-to-test labels Nov 21, 2022

coderzc requested review from mattisonchao and zymap November 21, 2022 09:50

coderzc requested a review from poorbarcode November 21, 2022 09:50

coderzc added 3 commits November 29, 2022 12:38

add BucketSnapshotSerializationException

3a248a0

BucketSnapshotException -> BucketSnapshotPersistenceException

711614a

improve key

3c9288c

poorbarcode approved these changes Dec 19, 2022

View reviewed changes

codelipenghui approved these changes Dec 19, 2022

View reviewed changes

zymap approved these changes Dec 19, 2022

View reviewed changes

mattisonchao reviewed Dec 19, 2022

View reviewed changes

Comment thread ...r/src/main/java/org/apache/pulsar/broker/delayed/bucket/BookkeeperBucketSnapshotStorage.java Outdated

mattisonchao reviewed Dec 19, 2022

View reviewed changes

Comment thread ...r/src/main/java/org/apache/pulsar/broker/delayed/bucket/BookkeeperBucketSnapshotStorage.java Outdated

mattisonchao reviewed Dec 19, 2022

View reviewed changes

codelipenghui reviewed Dec 19, 2022

View reviewed changes

coderzc force-pushed the bucket_delayed_store branch from 8a5c875 to 64b7310 Compare December 19, 2022 09:15

Clean up the created Ledger when add entry failed

2e6a8de

coderzc force-pushed the bucket_delayed_store branch from 64b7310 to 2e6a8de Compare December 19, 2022 09:17

mattisonchao approved these changes Dec 19, 2022

View reviewed changes

codelipenghui approved these changes Dec 20, 2022

View reviewed changes

codelipenghui merged commit fa8a5b2 into apache:master Dec 20, 2022

coderzc mentioned this pull request Mar 31, 2023

[feat][broker][PIP-195] Add metrics for bucket delayed message tracker #19716

Merged

5 tasks

Uh oh!

Conversation

coderzc commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

Documentation

Uh oh!

github-actions Bot commented Oct 24, 2022

Uh oh!

poorbarcode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

poorbarcode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mattisonchao Nov 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattisonchao Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coderzc commented Nov 18, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Nov 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codelipenghui left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zymap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattisonchao Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattisonchao left a comment

coderzc commented Sep 15, 2022 •

edited

Loading

mattisonchao Nov 18, 2022 •

edited

Loading

mattisonchao Dec 19, 2022 •

edited

Loading

codecov-commenter commented Nov 29, 2022 •

edited

Loading

mattisonchao Dec 19, 2022 •

edited

Loading