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/26 00:26:39 UTC
[GitHub] [pulsar] mattisonchao opened a new pull request, #15318: [improve][broker] Make some methods of `ClusterBase` pure async.
mattisonchao opened a new pull request, #15318:
URL: https://github.com/apache/pulsar/pull/15318
### Motivation
See PIP #14365 and change tracker #15043.
### Modifications
- Make `ClusterBase` `getClusters / getCluster / createCluster / updateCluster` 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 merged pull request #15318: [improve][broker] Make some methods of `ClusterBase` pure async.
Posted by GitBox <gi...@apache.org>.
mattisonchao merged PR #15318:
URL: https://github.com/apache/pulsar/pull/15318
--
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] gaoran10 commented on a diff in pull request #15318: [improve][broker] Make some methods of `ClusterBase` pure async.
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on code in PR #15318:
URL: https://github.com/apache/pulsar/pull/15318#discussion_r864431950
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/ClustersBase.java:
##########
@@ -158,27 +152,31 @@ public void createCluster(
+ "}"
)
)
- )
- ClusterDataImpl clusterData
- ) {
- validateSuperUserAccess();
- validatePoliciesReadOnlyAccess();
-
- try {
- NamedEntity.checkName(cluster);
- if (clusterResources().getCluster(cluster).isPresent()) {
- log.warn("[{}] Failed to create already existing cluster {}", clientAppId(), cluster);
- throw new RestException(Status.CONFLICT, "Cluster already exists");
- }
- clusterResources().createCluster(cluster, clusterData);
- log.info("[{}] Created cluster {}", clientAppId(), cluster);
- } catch (IllegalArgumentException e) {
- log.warn("[{}] Failed to create cluster with invalid name {}", clientAppId(), cluster, e);
- throw new RestException(Status.PRECONDITION_FAILED, "Cluster name is not valid");
- } catch (Exception e) {
- log.error("[{}] Failed to create cluster {}", clientAppId(), cluster, e);
- throw new RestException(e);
- }
+ ) ClusterDataImpl clusterData) {
+ validateSuperUserAccessAsync()
+ .thenCompose(__ -> validatePoliciesReadOnlyAccessAsync())
+ .thenCompose(__ -> {
+ NamedEntity.checkName(cluster);
+ return clusterResources().getClusterAsync(cluster);
+ }).thenCompose(clusterOpt -> {
+ if (clusterOpt.isPresent()) {
+ throw new RestException(Status.CONFLICT, "Cluster already exists");
Review Comment:
Ok
--
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 #15318: [improve][broker] Make some methods of `ClusterBase` pure async.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #15318:
URL: https://github.com/apache/pulsar/pull/15318#issuecomment-1110405197
/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 #15318: [improve][broker] Make some methods of `ClusterBase` pure async.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #15318:
URL: https://github.com/apache/pulsar/pull/15318#discussion_r858262637
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/ClustersBase.java:
##########
@@ -158,27 +151,25 @@ public void createCluster(
+ "}"
)
)
- )
- ClusterDataImpl clusterData
- ) {
- validateSuperUserAccess();
- validatePoliciesReadOnlyAccess();
-
- try {
- NamedEntity.checkName(cluster);
- if (clusterResources().getCluster(cluster).isPresent()) {
- log.warn("[{}] Failed to create already existing cluster {}", clientAppId(), cluster);
- throw new RestException(Status.CONFLICT, "Cluster already exists");
- }
- clusterResources().createCluster(cluster, clusterData);
- log.info("[{}] Created cluster {}", clientAppId(), cluster);
- } catch (IllegalArgumentException e) {
- log.warn("[{}] Failed to create cluster with invalid name {}", clientAppId(), cluster, e);
- throw new RestException(Status.PRECONDITION_FAILED, "Cluster name is not valid");
- } catch (Exception e) {
- log.error("[{}] Failed to create cluster {}", clientAppId(), cluster, e);
- throw new RestException(e);
- }
+ ) ClusterDataImpl clusterData) {
+ validateSuperUserAccessAsync()
+ .thenCompose(__ -> validatePoliciesReadOnlyAccessAsync())
+ .thenCompose(__ -> {
+ NamedEntity.checkName(cluster);
+ return clusterResources().getClusterAsync(cluster);
+ }).thenCompose(clusterOpt -> {
+ if (clusterOpt.isPresent()) {
+ throw new RestException(Status.CONFLICT, "Cluster already exists");
+ }
+ return clusterResources().createClusterAsync(cluster, clusterData);
+ }).thenAccept(__ -> {
+ log.info("[{}] Successfully to created cluster {}", clientAppId(), cluster);
Review Comment:
fixed.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/ClustersBase.java:
##########
@@ -211,22 +200,18 @@ public void updateCluster(
+ "}"
)
)
- )
- ClusterDataImpl clusterData
- ) {
- validateSuperUserAccess();
- validatePoliciesReadOnlyAccess();
-
- try {
- clusterResources().updateCluster(cluster, old -> clusterData);
- log.info("[{}] Updated cluster {}", clientAppId(), cluster);
- } catch (NotFoundException e) {
- log.warn("[{}] Failed to update cluster {}: Does not exist", clientAppId(), cluster);
- throw new RestException(Status.NOT_FOUND, "Cluster does not exist");
- } catch (Exception e) {
- log.error("[{}] Failed to update cluster {}", clientAppId(), cluster, e);
- throw new RestException(e);
- }
+ ) ClusterDataImpl clusterData) {
+ validateSuperUserAccessAsync()
+ .thenCompose(__ -> validatePoliciesReadOnlyAccessAsync())
+ .thenCompose(__ -> clusterResources().updateClusterAsync(cluster, old -> clusterData))
+ .thenAccept(__ -> {
+ log.info("[{}] Successfully to update cluster {}", clientAppId(), cluster);
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] mattisonchao commented on pull request #15318: [improve][broker] Make some methods of `ClusterBase` pure async.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #15318:
URL: https://github.com/apache/pulsar/pull/15318#issuecomment-1109236402
/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 pull request #15318: [improve][broker] Make some methods of `ClusterBase` pure async.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #15318:
URL: https://github.com/apache/pulsar/pull/15318#issuecomment-1111815105
/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 pull request #15318: [improve][broker] Make some methods of `ClusterBase` pure async.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #15318:
URL: https://github.com/apache/pulsar/pull/15318#issuecomment-1111815108
/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] gaoran10 commented on a diff in pull request #15318: [improve][broker] Make some methods of `ClusterBase` pure async.
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on code in PR #15318:
URL: https://github.com/apache/pulsar/pull/15318#discussion_r861425764
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/ClustersBase.java:
##########
@@ -158,27 +152,31 @@ public void createCluster(
+ "}"
)
)
- )
- ClusterDataImpl clusterData
- ) {
- validateSuperUserAccess();
- validatePoliciesReadOnlyAccess();
-
- try {
- NamedEntity.checkName(cluster);
- if (clusterResources().getCluster(cluster).isPresent()) {
- log.warn("[{}] Failed to create already existing cluster {}", clientAppId(), cluster);
- throw new RestException(Status.CONFLICT, "Cluster already exists");
- }
- clusterResources().createCluster(cluster, clusterData);
- log.info("[{}] Created cluster {}", clientAppId(), cluster);
- } catch (IllegalArgumentException e) {
- log.warn("[{}] Failed to create cluster with invalid name {}", clientAppId(), cluster, e);
- throw new RestException(Status.PRECONDITION_FAILED, "Cluster name is not valid");
- } catch (Exception e) {
- log.error("[{}] Failed to create cluster {}", clientAppId(), cluster, e);
- throw new RestException(e);
- }
+ ) ClusterDataImpl clusterData) {
+ validateSuperUserAccessAsync()
+ .thenCompose(__ -> validatePoliciesReadOnlyAccessAsync())
+ .thenCompose(__ -> {
+ NamedEntity.checkName(cluster);
+ return clusterResources().getClusterAsync(cluster);
+ }).thenCompose(clusterOpt -> {
+ if (clusterOpt.isPresent()) {
+ throw new RestException(Status.CONFLICT, "Cluster already exists");
Review Comment:
Do we need to retain the warning 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] Technoboy- commented on a diff in pull request #15318: [improve][broker] Make some methods of `ClusterBase` pure async.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15318:
URL: https://github.com/apache/pulsar/pull/15318#discussion_r858227831
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/ClustersBase.java:
##########
@@ -158,27 +151,25 @@ public void createCluster(
+ "}"
)
)
- )
- ClusterDataImpl clusterData
- ) {
- validateSuperUserAccess();
- validatePoliciesReadOnlyAccess();
-
- try {
- NamedEntity.checkName(cluster);
- if (clusterResources().getCluster(cluster).isPresent()) {
- log.warn("[{}] Failed to create already existing cluster {}", clientAppId(), cluster);
- throw new RestException(Status.CONFLICT, "Cluster already exists");
- }
- clusterResources().createCluster(cluster, clusterData);
- log.info("[{}] Created cluster {}", clientAppId(), cluster);
- } catch (IllegalArgumentException e) {
- log.warn("[{}] Failed to create cluster with invalid name {}", clientAppId(), cluster, e);
- throw new RestException(Status.PRECONDITION_FAILED, "Cluster name is not valid");
- } catch (Exception e) {
- log.error("[{}] Failed to create cluster {}", clientAppId(), cluster, e);
- throw new RestException(e);
- }
+ ) ClusterDataImpl clusterData) {
+ validateSuperUserAccessAsync()
+ .thenCompose(__ -> validatePoliciesReadOnlyAccessAsync())
+ .thenCompose(__ -> {
+ NamedEntity.checkName(cluster);
+ return clusterResources().getClusterAsync(cluster);
+ }).thenCompose(clusterOpt -> {
+ if (clusterOpt.isPresent()) {
+ throw new RestException(Status.CONFLICT, "Cluster already exists");
+ }
+ return clusterResources().createClusterAsync(cluster, clusterData);
+ }).thenAccept(__ -> {
+ log.info("[{}] Successfully to created cluster {}", clientAppId(), cluster);
Review Comment:
`Successfully to created cluster` -> `Created cluster`
Keep the original 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 #15318: [improve][broker] Make some methods of `ClusterBase` pure async.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on PR #15318:
URL: https://github.com/apache/pulsar/pull/15318#issuecomment-1109389789
/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 #15318: [improve][broker] Make some methods of `ClusterBase` pure async.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15318:
URL: https://github.com/apache/pulsar/pull/15318#discussion_r858228664
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/ClustersBase.java:
##########
@@ -211,22 +200,18 @@ public void updateCluster(
+ "}"
)
)
- )
- ClusterDataImpl clusterData
- ) {
- validateSuperUserAccess();
- validatePoliciesReadOnlyAccess();
-
- try {
- clusterResources().updateCluster(cluster, old -> clusterData);
- log.info("[{}] Updated cluster {}", clientAppId(), cluster);
- } catch (NotFoundException e) {
- log.warn("[{}] Failed to update cluster {}: Does not exist", clientAppId(), cluster);
- throw new RestException(Status.NOT_FOUND, "Cluster does not exist");
- } catch (Exception e) {
- log.error("[{}] Failed to update cluster {}", clientAppId(), cluster, e);
- throw new RestException(e);
- }
+ ) ClusterDataImpl clusterData) {
+ validateSuperUserAccessAsync()
+ .thenCompose(__ -> validatePoliciesReadOnlyAccessAsync())
+ .thenCompose(__ -> clusterResources().updateClusterAsync(cluster, old -> clusterData))
+ .thenAccept(__ -> {
+ log.info("[{}] Successfully to update cluster {}", clientAppId(), cluster);
Review Comment:
Keep the original log.
`Successfully to update cluster` -> `Updated cluster`
--
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 #15318: [improve][broker] Make some methods of `ClusterBase` pure async.
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #15318:
URL: https://github.com/apache/pulsar/pull/15318#discussion_r862271481
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/ClustersBase.java:
##########
@@ -158,27 +152,31 @@ public void createCluster(
+ "}"
)
)
- )
- ClusterDataImpl clusterData
- ) {
- validateSuperUserAccess();
- validatePoliciesReadOnlyAccess();
-
- try {
- NamedEntity.checkName(cluster);
- if (clusterResources().getCluster(cluster).isPresent()) {
- log.warn("[{}] Failed to create already existing cluster {}", clientAppId(), cluster);
- throw new RestException(Status.CONFLICT, "Cluster already exists");
- }
- clusterResources().createCluster(cluster, clusterData);
- log.info("[{}] Created cluster {}", clientAppId(), cluster);
- } catch (IllegalArgumentException e) {
- log.warn("[{}] Failed to create cluster with invalid name {}", clientAppId(), cluster, e);
- throw new RestException(Status.PRECONDITION_FAILED, "Cluster name is not valid");
- } catch (Exception e) {
- log.error("[{}] Failed to create cluster {}", clientAppId(), cluster, e);
- throw new RestException(e);
- }
+ ) ClusterDataImpl clusterData) {
+ validateSuperUserAccessAsync()
+ .thenCompose(__ -> validatePoliciesReadOnlyAccessAsync())
+ .thenCompose(__ -> {
+ NamedEntity.checkName(cluster);
+ return clusterResources().getClusterAsync(cluster);
+ }).thenCompose(clusterOpt -> {
+ if (clusterOpt.isPresent()) {
+ throw new RestException(Status.CONFLICT, "Cluster already exists");
Review Comment:
In the original logic, they will print warn log and error log, I think it's duplicated
Because we have a global error log, I think we don't need to print a duplicate warn 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