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

[fix][broker] Topic could be in fenced state forever if deletion fails #19129

Merged
merged 4 commits into from
Jan 4, 2023

Conversation

nicoloboschi
Copy link
Contributor

@nicoloboschi nicoloboschi commented Jan 4, 2023

Motivation

There's blind spot in the topic deletion where, if an exception occurs during the partitioned metadata update, the topic ends in a fenced state and it can't be deleted or used. With a broker restarts, the topic is deletable again. However the topic might be in an inconsistent state.

Modifications

  • Removed fenced flag in the topic if an error occurs during the partition metadata update
  • Add unit test
  • (tests) Fix FaultInjectionMetadataStore to be usable also from the MetadataCache methods

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

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

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 4, 2023
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.

Nice catch!

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM

I left a minor comment, PTAL

@@ -1289,6 +1289,11 @@ public void deleteLedgerComplete(Object ctx) {
});

return deleteFuture;
}).whenComplete((value, ex) -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you could use 'exceptionally'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

exceptionally swallows the exception so it's not a good fit here

}).whenComplete((value, ex) -> {
if (ex != null) {
unfenceTopicToResume();
log.error("[{}] Error deleting topic", topic, 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 is better to log before calling unfenceTopicToResume

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@codecov-commenter
Copy link

codecov-commenter commented Jan 4, 2023

Codecov Report

Merging #19129 (4e39570) into master (feb3cb4) will increase coverage by 1.15%.
The diff coverage is 10.65%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #19129      +/-   ##
============================================
+ Coverage     46.35%   47.51%   +1.15%     
- Complexity     8939    10755    +1816     
============================================
  Files           597      712     +115     
  Lines         56858    69650   +12792     
  Branches       5905     7481    +1576     
============================================
+ Hits          26357    33094    +6737     
- Misses        27616    32842    +5226     
- Partials       2885     3714     +829     
Flag Coverage Δ
unittests 47.51% <10.65%> (+1.15%) ⬆️

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 50.90% <0.00%> (+4.61%) ⬆️
...va/org/apache/pulsar/client/impl/ConsumerImpl.java 15.06% <0.00%> (-0.03%) ⬇️
...he/pulsar/client/impl/MultiTopicsConsumerImpl.java 22.80% <0.00%> (+0.01%) ⬆️
...ache/pulsar/client/impl/ZeroQueueConsumerImpl.java 0.00% <0.00%> (ø)
...va/org/apache/pulsar/client/impl/ConsumerBase.java 21.51% <6.66%> (-0.42%) ⬇️
...va/org/apache/pulsar/broker/service/ServerCnx.java 49.63% <36.00%> (+0.66%) ⬆️
... and 190 more

@eolivelli eolivelli merged commit a6516a8 into apache:master Jan 4, 2023
nicoloboschi added a commit to datastax/pulsar that referenced this pull request Jan 4, 2023
liangyepianzhou pushed a commit that referenced this pull request Feb 7, 2023
@liangyepianzhou
Copy link
Contributor

So this PR does not need to cherry-pick to branch2.10, right?

@nicoloboschi I see the partitioned metadata update when topic deleted is introduced by #18193 that does not cherry-pick to branch-2.10. So this PR neither needs to cherry-pick to branch2.10, right?

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 release/2.11.5 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants