-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[fix][broker] Topic could be in fenced state forever if deletion fails #19129
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
There was a problem hiding this 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) -> { |
There was a problem hiding this comment.
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'
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
apache#19129) (cherry picked from commit a6516a8)
@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? |
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
FaultInjectionMetadataStore
to be usable also from the MetadataCache methodsVerifying this change
Documentation
doc
doc-required
doc-not-needed
doc-complete