-
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] Fix namespace backlog quota check with retention. #17706
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.
LGTM. Thanks @Jason918!
@@ -370,17 +370,21 @@ protected CompletableFuture<Optional<TopicPolicies>> getTopicPoliciesAsyncWithRe | |||
} | |||
|
|||
protected boolean checkBacklogQuota(BacklogQuota quota, RetentionPolicies retention) { | |||
if (retention == null || retention.getRetentionSizeInMB() <= 0 || retention.getRetentionTimeInMinutes() <= 0) { | |||
if (retention == null | |||
|| (retention.getRetentionSizeInMB() <= 0 && retention.getRetentionTimeInMinutes() <= 0)) { |
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.
Looks like the added test can't cover this branch?
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.
@codelipenghui As mentioned in the issue. The test in L3417 won't fail before this change.
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.
And the case of (retention.getRetentionSizeInMB() <= 0 && retention.getRetentionTimeInMinutes() <= 0)
is the default setting, it's covered by previous unit tests.
(cherry picked from commit c6967cd)
(cherry picked from commit c6967cd)
(cherry picked from commit c6967cd)
Fixes #17707
Motivation
checkBacklogQuota
fails when both retention.size and backlogQuota.size is -1. Same as time check.Verifying this change
This change added tests and can be verified as follows:
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
doc-required
(Your PR needs to update docs and you will update later)
doc-not-needed
bug
doc
(Your PR contains doc changes)
doc-complete
(Docs have been already added)