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] [admin] setOffloadThreshold response before it's finished #19370

Merged

Conversation

poorbarcode
Copy link
Contributor

@poorbarcode poorbarcode commented Jan 31, 2023

Fixes #19366

Motivation

Admin API setOffloadThreshold response before it is finished.

You can reproduce by AdminApiOffloadTest.testSetNamespaceOffloadPolicies, If you append sleep(1000) after this line(

NotificationType type = stat.getVersion() == 0 ? NotificationType.Created
), the chances of problems occurring increase to 100%

Modifications

response after it is finishied

Documentation

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

Matching PR in forked repository

PR in forked repository:

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 31, 2023
@poorbarcode
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@@ -2047,7 +2047,7 @@ protected CompletableFuture<Void> internalSetOffloadThresholdInSecondsAsync(long

validateNamespacePolicyOperationAsync(namespaceName, PolicyName.OFFLOAD, PolicyOperation.WRITE)
.thenApply(v -> validatePoliciesReadOnlyAccessAsync())
Copy link
Member

@lhotari lhotari Feb 1, 2023

Choose a reason for hiding this comment

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

@poorbarcode Thanks for finding the issue and fixing this.

Isn't there a similar bug on the previous line with the validatePoliciesReadOnlyAccessAsync call?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, you are right. Thanks for helping to check. I have submitted a PR to fix it.

@liangyepianzhou
Copy link
Contributor

This change is to fix the admin API by adding in the below PR which is not included in branch-2.10. So remove the label release 2.10.4.
#18218

@Technoboy-
Copy link
Contributor

No need to cherry pick to branch-2.11 too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs ready-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky-test: AdminApiOffloadTest.testSetNamespaceOffloadPolicies
5 participants