[feat][broker][PIP-195] Implement BookkeeperBucketSnapshotStorage - part3 #17677

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

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

...ar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketSnapshotException.java

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

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);

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

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

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

+    @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?

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

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

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

+                    } 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 )

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

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

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

+                    } 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.

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

…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

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

+                                                        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

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

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

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

+                                                        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

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

@@ -61,7 +61,11 @@ public CompletableFuture<Long> createBucketSnapshot(SnapshotMetadata snapshotMet
                .thenCompose(ledgerHandle -> addEntry(ledgerHandle, snapshotMetadata.toByteArray())
                        .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

...r/src/main/java/org/apache/pulsar/broker/delayed/bucket/BookkeeperBucketSnapshotStorage.java Outdated Show resolved Hide resolved

improve code

475d108

poorbarcode approved these changes Nov 14, 2022

View reviewed changes

...r/src/main/java/org/apache/pulsar/broker/delayed/bucket/BookkeeperBucketSnapshotStorage.java Show resolved Hide resolved

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

...r/src/main/java/org/apache/pulsar/broker/delayed/bucket/BookkeeperBucketSnapshotStorage.java Outdated Show resolved Hide resolved

coderzc commented Nov 21, 2022

View reviewed changes

...r/src/main/java/org/apache/pulsar/broker/delayed/bucket/BookkeeperBucketSnapshotStorage.java Outdated Show resolved Hide resolved

...r/src/main/java/org/apache/pulsar/broker/delayed/bucket/BookkeeperBucketSnapshotStorage.java Outdated Show resolved Hide resolved

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

...r/src/main/java/org/apache/pulsar/broker/delayed/bucket/BookkeeperBucketSnapshotStorage.java Outdated Show resolved Hide resolved

mattisonchao reviewed Dec 19, 2022

View reviewed changes

...r/src/main/java/org/apache/pulsar/broker/delayed/bucket/BookkeeperBucketSnapshotStorage.java Outdated Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat][broker][PIP-195] Implement BookkeeperBucketSnapshotStorage - part3 #17677

[feat][broker][PIP-195] Implement BookkeeperBucketSnapshotStorage - part3 #17677

coderzc commented Sep 15, 2022 •

edited

Loading

github-actions bot commented Oct 24, 2022

poorbarcode left a comment

poorbarcode left a comment

mattisonchao Nov 18, 2022 •

edited

Loading

mattisonchao Nov 18, 2022

coderzc Nov 18, 2022

mattisonchao Dec 19, 2022 •

edited

Loading

zymap Nov 18, 2022

coderzc Nov 18, 2022

coderzc commented Nov 18, 2022

codecov-commenter commented Nov 29, 2022 •

edited

Loading

codelipenghui left a comment

codelipenghui Dec 19, 2022

coderzc Dec 19, 2022

zymap left a comment

mattisonchao Dec 19, 2022

coderzc Dec 19, 2022

mattisonchao Dec 19, 2022 •

edited

Loading

codelipenghui Dec 19, 2022

coderzc Dec 19, 2022

mattisonchao left a comment

[feat][broker][PIP-195] Implement BookkeeperBucketSnapshotStorage - part3 #17677

[feat][broker][PIP-195] Implement BookkeeperBucketSnapshotStorage - part3 #17677

Conversation

coderzc commented Sep 15, 2022 • edited Loading

Motivation

Modifications

Verifying this change

Documentation

github-actions bot commented Oct 24, 2022

poorbarcode left a comment

Choose a reason for hiding this comment

poorbarcode left a comment

Choose a reason for hiding this comment

mattisonchao Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattisonchao Dec 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderzc commented Nov 18, 2022

codecov-commenter commented Nov 29, 2022 • edited Loading

Codecov Report

codelipenghui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zymap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattisonchao Dec 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattisonchao left a comment

Choose a reason for hiding this 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