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

[improve][broker] Make BucketDelayedDeliveryTracker can retry snapshot operation & improve logs #19577

Merged
merged 15 commits into from
Mar 2, 2023
Merged

[improve][broker] Make BucketDelayedDeliveryTracker can retry snapshot operation & improve logs #19577

merged 15 commits into from
Mar 2, 2023

Conversation

coderzc
Copy link
Member

@coderzc coderzc commented Feb 21, 2023 •
edited
Loading

PIP: #16763

Motivation & Modifications

  • When bucket snapshot create/load/delete is failed, use the executeWithRetry method to auto retry operation.
  • Back bucket state and ignore load exception to reload this segment on the next schedule.
  • Put indexes back into the shared queue and downgrade to memory mode when create bucket snapshot failed.
  • Improve logs
  • Add test
  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

@coderzc coderzc self-assigned this Feb 21, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Feb 21, 2023
@coderzc coderzc added this to the 3.0.0 milestone Feb 21, 2023
@coderzc coderzc requested a review from mattisonchao February 21, 2023 07:24
@coderzc coderzc force-pushed the improve_bucket_retry_op branch 2 times, most recently from 7e5a16c to af32f57 Compare February 21, 2023 07:47
315157973
315157973 reviewed Feb 21, 2023
View reviewed changes
Copy link
Contributor

@315157973 315157973 left a comment

Choose a reason for hiding this comment

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

I think there are still some unit tests missing

@coderzc coderzc force-pushed the improve_bucket_retry_op branch 4 times, most recently from 623d557 to b47aa6f Compare February 23, 2023 08:08
@coderzc
Copy link
Member Author

coderzc commented Feb 23, 2023

I think there are still some unit tests missing

@315157973 I already add unit tests to cover the change, please take a look.

@coderzc coderzc force-pushed the improve_bucket_retry_op branch from b47aa6f to f1566c8 Compare February 23, 2023 08:18
@coderzc coderzc force-pushed the improve_bucket_retry_op branch from 05f1c73 to 4c6b0b4 Compare February 23, 2023 09:48
@coderzc coderzc changed the title [improve][broker] Auto retry bucket snapshot create/load/delete operation [improve][broker] Auto retry bucket snapshot create/load/delete operation & improve logs Feb 23, 2023
@coderzc coderzc force-pushed the improve_bucket_retry_op branch from 4c6b0b4 to 82178f0 Compare February 23, 2023 10:09
@coderzc coderzc changed the title [improve][broker] Auto retry bucket snapshot create/load/delete operation & improve logs [improve][broker] Make BucketDelayedDeliveryTracker can retry snapshot operation & improve logs Feb 23, 2023
codelipenghui
codelipenghui reviewed Feb 27, 2023
View reviewed changes
...oker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java Outdated
return asyncMergeBucketSnapshot(values.get(minIndex), values.get(minIndex + 1));
ImmutableBucket immutableBucketA = values.get(minIndex);
ImmutableBucket immutableBucketB = values.get(minIndex + 1);
log.info("[{}] Merging bucket snapshot, bucketAKey: {}, bucketBKey: {}", dispatcher.getName(),
Copy link
Contributor

Choose a reason for hiding this comment

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

We should change to debug level since it will make a lot of noise if you have many topics on the broker.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok.

...oker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java Outdated
log.debug("[{}] Load next snapshot segment, bucket: {}", dispatcher.getName(), bucket);
}
final int lastSegmentEntryId = bucket.currentSegmentEntryId;
log.info("[{}] Loading next bucket snapshot segment, bucketKey: {}, nextSegmentEntryId: {}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change to debug level?

@coderzc coderzc force-pushed the improve_bucket_retry_op branch from c0184a7 to 2316e13 Compare February 27, 2023 10:36
315157973
315157973 reviewed Feb 27, 2023
View reviewed changes
...r/src/main/java/org/apache/pulsar/broker/delayed/bucket/BookkeeperBucketSnapshotStorage.java
@@ -154,7 +154,7 @@ private CompletableFuture<LedgerHandle> createLedger(String bucketKey) {
LedgerPassword,
(rc, handle, ctx) -> {
if (rc != BKException.Code.OK) {
future.completeExceptionally(bkException("Failed to create ledger", rc, -1));
future.completeExceptionally(bkException("Create ledger", rc, -1));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why modify these logs, the previous description looks more accurate

Copy link
Member Author

Choose a reason for hiding this comment

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

This field is the name of the operation.

pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/Bucket.java Outdated

private volatile Long bucketId;

private volatile CompletableFuture<Long> snapshotCreateFuture;


Bucket(ManagedCursor cursor, BucketSnapshotStorage storage, long startLedgerId, long endLedgerId) {
this(cursor, storage, startLedgerId, endLedgerId, new HashMap<>(), -1, -1, 0, 0, null, null);
Bucket(PersistentDispatcherMultipleConsumers dispatcher,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change to dispatch, just to get the name of dispatch and print log?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, I need print name of dispatcher

Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't need to access other APIs of the dispatcher, we'd better only pass the topic name and subscription name to the Bucket.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I improve it.

coderzc added 2 commits March 1, 2023 09:56
@coderzc
Merge branch 'master' into improve_bucket_retry_op
1e0ef19 
# Conflicts:
#	pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTracker.java
#	pulsar-broker/src/main/java/org/apache/pulsar/broker/delayed/bucket/ImmutableBucket.java
#	pulsar-broker/src/test/java/org/apache/pulsar/broker/delayed/bucket/BucketDelayedDeliveryTrackerTest.java
@coderzc
improve code
99e6ee9
@coderzc coderzc force-pushed the improve_bucket_retry_op branch from 6db0a93 to 99e6ee9 Compare March 1, 2023 02:44
@coderzc coderzc force-pushed the improve_bucket_retry_op branch 4 times, most recently from 4916f53 to a184148 Compare March 1, 2023 07:21
@coderzc coderzc force-pushed the improve_bucket_retry_op branch 6 times, most recently from 60e1eae to 51de068 Compare March 1, 2023 09:10
@coderzc coderzc force-pushed the improve_bucket_retry_op branch from 51de068 to be9092b Compare March 1, 2023 09:23
@codelipenghui codelipenghui merged commit 5e717c6 into apache:master Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codelipenghui codelipenghui codelipenghui approved these changes

@315157973 315157973 315157973 approved these changes

@mattisonchao mattisonchao Awaiting requested review from mattisonchao

Assignees

@coderzc coderzc

Labels
area/broker doc-not-needed Your PR changes do not impact docs ready-to-test
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@coderzc @315157973 @codelipenghui