You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/04/24 00:07:41 UTC
[GitHub] [pulsar] mattisonchao opened a new pull request, #15281: [improve][broker] PIP-149 Make some methods of `BrokersBase` pure async.
mattisonchao opened a new pull request, #15281:
URL: https://github.com/apache/pulsar/pull/15281
### Motivation
See PIP #14365 and change tracker #15043.
### Modifications
- Make `BrokersBase` `getRuntimeConfiguration`/`getInternalConfigurationData`/`backlogQuotaCheck` methods to pure async.
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Documentation
- [x] `no-need-doc`
(Please explain why)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on a diff in pull request #15281: [improve][broker] Make some methods of `BrokersBase` pure async.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #15281:
URL: https://github.com/apache/pulsar/pull/15281#discussion_r858258840
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java:
##########
@@ -286,16 +295,17 @@ public InternalConfigurationData getInternalConfigurationData() {
@ApiResponse(code = 403, message = "Don't have admin permission"),
@ApiResponse(code = 500, message = "Internal server error")})
public void backlogQuotaCheck(@Suspended AsyncResponse asyncResponse) {
- validateSuperUserAccess();
- pulsar().getBrokerService().getBacklogQuotaChecker().execute(safeRun(()->{
- try {
- pulsar().getBrokerService().monitorBacklogQuota();
- asyncResponse.resume(Response.noContent().build());
- } catch (Exception e) {
- LOG.error("trigger backlogQuotaCheck fail", e);
- asyncResponse.resume(new RestException(e));
- }
- }));
+ validateSuperUserAccessAsync()
+ .thenAcceptAsync(__ -> {
+ pulsar().getBrokerService().monitorBacklogQuota();
+ LOG.info("[{}] Successfully to trigger backlog quota check.", clientAppId());
Review Comment:
The log is needed here because this is an action that needs to remind the user whether the operation was successful.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on a diff in pull request #15281: [improve][broker] Make some methods of `BrokersBase` pure async.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15281:
URL: https://github.com/apache/pulsar/pull/15281#discussion_r857505656
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java:
##########
@@ -237,9 +236,16 @@ public List<String> getDynamicConfigurationName() {
@Path("/configuration/runtime")
@ApiOperation(value = "Get all runtime configurations. This operation requires Pulsar super-user privileges.")
@ApiResponses(value = { @ApiResponse(code = 403, message = "Don't have admin permission") })
- public Map<String, String> getRuntimeConfiguration() {
- validateSuperUserAccess();
- return pulsar().getBrokerService().getRuntimeConfiguration();
+ public void getRuntimeConfiguration(@Suspended AsyncResponse asyncResponse) {
+ validateSuperUserAccessAsync()
+ .thenAccept(__ -> {
+ LOG.info("[{}] Successfully to get runtime configuration.", clientAppId());
Review Comment:
I'm wondering if we need this successful log.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #15281: [improve][broker] Make some methods of `BrokersBase` pure async.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #15281:
URL: https://github.com/apache/pulsar/pull/15281#issuecomment-1109236134
/pulsarbot rerun-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on a diff in pull request #15281: [improve][broker] Make some methods of `BrokersBase` pure async.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15281:
URL: https://github.com/apache/pulsar/pull/15281#discussion_r858232445
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java:
##########
@@ -286,16 +295,17 @@ public InternalConfigurationData getInternalConfigurationData() {
@ApiResponse(code = 403, message = "Don't have admin permission"),
@ApiResponse(code = 500, message = "Internal server error")})
public void backlogQuotaCheck(@Suspended AsyncResponse asyncResponse) {
- validateSuperUserAccess();
- pulsar().getBrokerService().getBacklogQuotaChecker().execute(safeRun(()->{
- try {
- pulsar().getBrokerService().monitorBacklogQuota();
- asyncResponse.resume(Response.noContent().build());
- } catch (Exception e) {
- LOG.error("trigger backlogQuotaCheck fail", e);
- asyncResponse.resume(new RestException(e));
- }
- }));
+ validateSuperUserAccessAsync()
+ .thenAcceptAsync(__ -> {
+ pulsar().getBrokerService().monitorBacklogQuota();
+ LOG.info("[{}] Successfully to trigger backlog quota check.", clientAppId());
Review Comment:
Forget to remove this log.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on a diff in pull request #15281: [improve][broker] Make some methods of `BrokersBase` pure async.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #15281:
URL: https://github.com/apache/pulsar/pull/15281#discussion_r858273889
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java:
##########
@@ -286,16 +295,17 @@ public InternalConfigurationData getInternalConfigurationData() {
@ApiResponse(code = 403, message = "Don't have admin permission"),
@ApiResponse(code = 500, message = "Internal server error")})
public void backlogQuotaCheck(@Suspended AsyncResponse asyncResponse) {
- validateSuperUserAccess();
- pulsar().getBrokerService().getBacklogQuotaChecker().execute(safeRun(()->{
- try {
- pulsar().getBrokerService().monitorBacklogQuota();
- asyncResponse.resume(Response.noContent().build());
- } catch (Exception e) {
- LOG.error("trigger backlogQuotaCheck fail", e);
- asyncResponse.resume(new RestException(e));
- }
- }));
+ validateSuperUserAccessAsync()
+ .thenAcceptAsync(__ -> {
+ pulsar().getBrokerService().monitorBacklogQuota();
+ LOG.info("[{}] Successfully to trigger backlog quota check.", clientAppId());
Review Comment:
fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- commented on a diff in pull request #15281: [improve][broker] Make some methods of `BrokersBase` pure async.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15281:
URL: https://github.com/apache/pulsar/pull/15281#discussion_r857505822
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java:
##########
@@ -237,9 +236,16 @@ public List<String> getDynamicConfigurationName() {
@Path("/configuration/runtime")
@ApiOperation(value = "Get all runtime configurations. This operation requires Pulsar super-user privileges.")
@ApiResponses(value = { @ApiResponse(code = 403, message = "Don't have admin permission") })
- public Map<String, String> getRuntimeConfiguration() {
- validateSuperUserAccess();
- return pulsar().getBrokerService().getRuntimeConfiguration();
+ public void getRuntimeConfiguration(@Suspended AsyncResponse asyncResponse) {
+ validateSuperUserAccessAsync()
+ .thenAccept(__ -> {
+ LOG.info("[{}] Successfully to get runtime configuration.", clientAppId());
Review Comment:
@codelipenghui @Jason918
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #15281: [improve][broker] Make some methods of `BrokersBase` pure async.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #15281:
URL: https://github.com/apache/pulsar/pull/15281#issuecomment-1110351486
/pulsarbot rerun-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on a diff in pull request #15281: [improve][broker] Make some methods of `BrokersBase` pure async.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #15281:
URL: https://github.com/apache/pulsar/pull/15281#discussion_r858115420
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java:
##########
@@ -237,9 +236,16 @@ public List<String> getDynamicConfigurationName() {
@Path("/configuration/runtime")
@ApiOperation(value = "Get all runtime configurations. This operation requires Pulsar super-user privileges.")
@ApiResponses(value = { @ApiResponse(code = 403, message = "Don't have admin permission") })
- public Map<String, String> getRuntimeConfiguration() {
- validateSuperUserAccess();
- return pulsar().getBrokerService().getRuntimeConfiguration();
+ public void getRuntimeConfiguration(@Suspended AsyncResponse asyncResponse) {
+ validateSuperUserAccessAsync()
+ .thenAccept(__ -> {
+ LOG.info("[{}] Successfully to get runtime configuration.", clientAppId());
Review Comment:
Looks like we will print the server request log. So, the log for the get method is useless here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on a diff in pull request #15281: [improve][broker] Make some methods of `BrokersBase` pure async.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #15281:
URL: https://github.com/apache/pulsar/pull/15281#discussion_r858124095
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/BrokersBase.java:
##########
@@ -237,9 +236,16 @@ public List<String> getDynamicConfigurationName() {
@Path("/configuration/runtime")
@ApiOperation(value = "Get all runtime configurations. This operation requires Pulsar super-user privileges.")
@ApiResponses(value = { @ApiResponse(code = 403, message = "Don't have admin permission") })
- public Map<String, String> getRuntimeConfiguration() {
- validateSuperUserAccess();
- return pulsar().getBrokerService().getRuntimeConfiguration();
+ public void getRuntimeConfiguration(@Suspended AsyncResponse asyncResponse) {
+ validateSuperUserAccessAsync()
+ .thenAccept(__ -> {
+ LOG.info("[{}] Successfully to get runtime configuration.", clientAppId());
Review Comment:
fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Technoboy- merged pull request #15281: [improve][broker] Make some methods of `BrokersBase` pure async.
Posted by GitBox <gi...@apache.org>.
Technoboy- merged PR #15281:
URL: https://github.com/apache/pulsar/pull/15281
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on pull request #15281: [improve][broker] Make some methods of `BrokersBase` pure async.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #15281:
URL: https://github.com/apache/pulsar/pull/15281#issuecomment-1108339199
/pulsarbot rerun-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org