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

[improve][broker][PIP-149]make setPersistence method async in Namespaces #17421

Merged
merged 5 commits into from
Sep 11, 2022

Conversation

Pomelongan
Copy link
Contributor

Motivation

See #14365

Verifying this change

  • Make sure that the change passes the CI checks.
    This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

Check the box below or label this PR directly.

Need to update docs?

  • doc-not-needed

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Sep 2, 2022
@Pomelongan
Copy link
Contributor Author

/pulsarbot run-failure-checks

return null;
});
})
.thenCompose(__ -> doUpdatePersistenceAsync(null));
Copy link
Contributor

Choose a reason for hiding this comment

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

doUpdatePersistenceAsync(null) -> doUpdatePersistenceAsync(persistence)

Comment on lines 1566 to 1571
.thenCompose(__ -> {
return CompletableFuture.supplyAsync(() -> {
validatePersistencePolicies(persistence);
return null;
});
})
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should remove this code block from line1566 to line1571

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AnonHxy PTAL

@Pomelongan
Copy link
Contributor Author

/pulsarbot run-failure-checks

@Technoboy- Technoboy- added this to the 2.12.0 milestone Sep 5, 2022
@Technoboy- Technoboy- added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/admin labels Sep 5, 2022
@AnonHxy
Copy link
Contributor

AnonHxy commented Sep 5, 2022

Please fix the CI org.apache.pulsar.broker.admin.NamespacesTest#testPersistenceUnauthorized @Pomelongan

@Pomelongan
Copy link
Contributor Author

/pulsarbot run-failure-checks

@Technoboy-
Copy link
Contributor

Please fix the check style issue.

@Pomelongan
Copy link
Contributor Author

Please fix the check style issue.
@Technoboy- PTAL~

@Pomelongan
Copy link
Contributor Author

/pulsarbot run-failure-checks

@AnonHxy AnonHxy merged commit 7bb290c into apache:master Sep 11, 2022
tisonkun pushed a commit to tisonkun/pulsar that referenced this pull request Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/admin doc-not-needed Your PR changes do not impact docs type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants