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/05/14 15:30:06 UTC
[GitHub] [pulsar] Technoboy- opened a new pull request, #15603: Make some methods in TenantsBase async.
Technoboy- opened a new pull request, #15603:
URL: https://github.com/apache/pulsar/pull/15603
### Motivation
See PIP #14365 and change tracker #15043.
Make `getTenants` / `getTenantAdmin` / `createTenant` / `updateTenant` / `deleteTenant` methods in TenantsBase to pure async.
### 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] nodece commented on a diff in pull request #15603: [improve][broker] Make some methods in TenantsBase async.
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #15603:
URL: https://github.com/apache/pulsar/pull/15603#discussion_r873051695
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java:
##########
@@ -90,23 +84,20 @@ public void getTenants(@Suspended final AsyncResponse asyncResponse) {
@ApiResponse(code = 404, message = "Tenant does not exist")})
public void getTenantAdmin(@Suspended final AsyncResponse asyncResponse,
@ApiParam(value = "The tenant name") @PathParam("tenant") String tenant) {
- final String clientAppId = clientAppId();
- try {
- validateSuperUserAccess();
- } catch (Exception e) {
- asyncResponse.resume(e);
- }
-
- tenantResources().getTenantAsync(tenant).whenComplete((tenantInfo, e) -> {
- if (e != null) {
- log.error("[{}] Failed to get Tenant {}", clientAppId, e.getMessage());
- asyncResponse.resume(new RestException(Status.INTERNAL_SERVER_ERROR, "Failed to get Tenant"));
- return;
- }
- boolean response = tenantInfo.isPresent() ? asyncResponse.resume(tenantInfo.get())
- : asyncResponse.resume(new RestException(Status.NOT_FOUND, "Tenant does not exist"));
- return;
- });
+ validateSuperUserAccessAsync()
+ .thenCompose(__ -> tenantResources().getTenantAsync(tenant))
+ .thenApply(tenantInfo -> {
+ if (!tenantInfo.isPresent()) {
+ new RestException(Status.NOT_FOUND, "Tenant does not exist");
+ }
+ return tenantInfo.get();
+ })
+ .thenAccept(asyncResponse::resume)
+ .exceptionally(ex -> {
+ log.error("[{}] Failed to get tenant admin {}", clientAppId(), ex);
Review Comment:
If the tenant does not exist, we don't need to log an error.
--
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] codelipenghui commented on a diff in pull request #15603: [improve][broker] Make some methods in TenantsBase async.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #15603:
URL: https://github.com/apache/pulsar/pull/15603#discussion_r873289772
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java:
##########
@@ -63,24 +63,18 @@ public class TenantsBase extends PulsarWebResource {
@ApiResponses(value = {@ApiResponse(code = 403, message = "The requester doesn't have admin permissions"),
@ApiResponse(code = 404, message = "Tenant doesn't exist")})
public void getTenants(@Suspended final AsyncResponse asyncResponse) {
- final String clientAppId = clientAppId();
- try {
- validateSuperUserAccess();
- } catch (Exception e) {
- asyncResponse.resume(e);
- return;
- }
- tenantResources().listTenantsAsync().whenComplete((tenants, e) -> {
- if (e != null) {
- log.error("[{}] Failed to get tenants list", clientAppId, e);
- asyncResponse.resume(new RestException(e));
- return;
- }
- // deep copy the tenants to avoid concurrent sort exception
- List<String> deepCopy = new ArrayList<>(tenants);
- deepCopy.sort(null);
- asyncResponse.resume(deepCopy);
- });
+ validateSuperUserAccessAsync()
+ .thenCompose(__ -> tenantResources().listTenantsAsync())
+ .thenAccept(tenants -> {
+ // deep copy the tenants to avoid concurrent sort exception
+ List<String> deepCopy = new ArrayList<>(tenants);
+ deepCopy.sort(null);
+ asyncResponse.resume(deepCopy);
Review Comment:
```suggestion
asyncResponse.resume(Collections.unmodifiableList(deepCopy));
```
--
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 #15603: [improve][broker] Make some methods in TenantsBase async.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15603:
URL: https://github.com/apache/pulsar/pull/15603#discussion_r873303305
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java:
##########
@@ -63,24 +63,18 @@ public class TenantsBase extends PulsarWebResource {
@ApiResponses(value = {@ApiResponse(code = 403, message = "The requester doesn't have admin permissions"),
@ApiResponse(code = 404, message = "Tenant doesn't exist")})
public void getTenants(@Suspended final AsyncResponse asyncResponse) {
- final String clientAppId = clientAppId();
- try {
- validateSuperUserAccess();
- } catch (Exception e) {
- asyncResponse.resume(e);
- return;
- }
- tenantResources().listTenantsAsync().whenComplete((tenants, e) -> {
- if (e != null) {
- log.error("[{}] Failed to get tenants list", clientAppId, e);
- asyncResponse.resume(new RestException(e));
- return;
- }
- // deep copy the tenants to avoid concurrent sort exception
- List<String> deepCopy = new ArrayList<>(tenants);
- deepCopy.sort(null);
- asyncResponse.resume(deepCopy);
- });
+ validateSuperUserAccessAsync()
+ .thenCompose(__ -> tenantResources().listTenantsAsync())
+ .thenAccept(tenants -> {
+ // deep copy the tenants to avoid concurrent sort exception
+ List<String> deepCopy = new ArrayList<>(tenants);
+ deepCopy.sort(null);
+ asyncResponse.resume(deepCopy);
+ }).exceptionally(ex -> {
+ log.error("[{}] Failed to get tenants list", clientAppId(), ex);
Review Comment:
Got it. I will define this at the beginning.
--
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 #15603: [improve][broker] Make some methods in TenantsBase async.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15603:
URL: https://github.com/apache/pulsar/pull/15603#discussion_r873090026
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java:
##########
@@ -90,23 +84,20 @@ public void getTenants(@Suspended final AsyncResponse asyncResponse) {
@ApiResponse(code = 404, message = "Tenant does not exist")})
public void getTenantAdmin(@Suspended final AsyncResponse asyncResponse,
@ApiParam(value = "The tenant name") @PathParam("tenant") String tenant) {
- final String clientAppId = clientAppId();
- try {
- validateSuperUserAccess();
- } catch (Exception e) {
- asyncResponse.resume(e);
- }
-
- tenantResources().getTenantAsync(tenant).whenComplete((tenantInfo, e) -> {
- if (e != null) {
- log.error("[{}] Failed to get Tenant {}", clientAppId, e.getMessage());
- asyncResponse.resume(new RestException(Status.INTERNAL_SERVER_ERROR, "Failed to get Tenant"));
- return;
- }
- boolean response = tenantInfo.isPresent() ? asyncResponse.resume(tenantInfo.get())
- : asyncResponse.resume(new RestException(Status.NOT_FOUND, "Tenant does not exist"));
- return;
- });
+ validateSuperUserAccessAsync()
+ .thenCompose(__ -> tenantResources().getTenantAsync(tenant))
+ .thenApply(tenantInfo -> {
+ if (!tenantInfo.isPresent()) {
+ new RestException(Status.NOT_FOUND, "Tenant does not exist");
Review Comment:
Oh, yes. 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] gaoran10 commented on a diff in pull request #15603: [improve][broker] Make some methods in TenantsBase async.
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on code in PR #15603:
URL: https://github.com/apache/pulsar/pull/15603#discussion_r873300633
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java:
##########
@@ -63,24 +63,18 @@ public class TenantsBase extends PulsarWebResource {
@ApiResponses(value = {@ApiResponse(code = 403, message = "The requester doesn't have admin permissions"),
@ApiResponse(code = 404, message = "Tenant doesn't exist")})
public void getTenants(@Suspended final AsyncResponse asyncResponse) {
- final String clientAppId = clientAppId();
- try {
- validateSuperUserAccess();
- } catch (Exception e) {
- asyncResponse.resume(e);
- return;
- }
- tenantResources().listTenantsAsync().whenComplete((tenants, e) -> {
- if (e != null) {
- log.error("[{}] Failed to get tenants list", clientAppId, e);
- asyncResponse.resume(new RestException(e));
- return;
- }
- // deep copy the tenants to avoid concurrent sort exception
- List<String> deepCopy = new ArrayList<>(tenants);
- deepCopy.sort(null);
- asyncResponse.resume(deepCopy);
- });
+ validateSuperUserAccessAsync()
+ .thenCompose(__ -> tenantResources().listTenantsAsync())
+ .thenAccept(tenants -> {
+ // deep copy the tenants to avoid concurrent sort exception
+ List<String> deepCopy = new ArrayList<>(tenants);
+ deepCopy.sort(null);
+ asyncResponse.resume(deepCopy);
+ }).exceptionally(ex -> {
+ log.error("[{}] Failed to get tenants list", clientAppId(), ex);
Review Comment:
I'm not sure if the `HttpRequest` object will be reused in async way, if reused the client appid may be modified.
--
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 #15603: [improve][broker] Make some methods in TenantsBase async.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15603:
URL: https://github.com/apache/pulsar/pull/15603#discussion_r873302412
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java:
##########
@@ -63,24 +63,18 @@ public class TenantsBase extends PulsarWebResource {
@ApiResponses(value = {@ApiResponse(code = 403, message = "The requester doesn't have admin permissions"),
@ApiResponse(code = 404, message = "Tenant doesn't exist")})
public void getTenants(@Suspended final AsyncResponse asyncResponse) {
- final String clientAppId = clientAppId();
- try {
- validateSuperUserAccess();
- } catch (Exception e) {
- asyncResponse.resume(e);
- return;
- }
- tenantResources().listTenantsAsync().whenComplete((tenants, e) -> {
- if (e != null) {
- log.error("[{}] Failed to get tenants list", clientAppId, e);
- asyncResponse.resume(new RestException(e));
- return;
- }
- // deep copy the tenants to avoid concurrent sort exception
- List<String> deepCopy = new ArrayList<>(tenants);
- deepCopy.sort(null);
- asyncResponse.resume(deepCopy);
- });
+ validateSuperUserAccessAsync()
+ .thenCompose(__ -> tenantResources().listTenantsAsync())
+ .thenAccept(tenants -> {
+ // deep copy the tenants to avoid concurrent sort exception
+ List<String> deepCopy = new ArrayList<>(tenants);
+ deepCopy.sort(null);
+ asyncResponse.resume(deepCopy);
Review Comment:
Ah, this seems ok, we return a new list to the user, so we don't care the result, right ?
--
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 #15603: [improve][broker] Make some methods in TenantsBase async.
Posted by GitBox <gi...@apache.org>.
Technoboy- merged PR #15603:
URL: https://github.com/apache/pulsar/pull/15603
--
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] codelipenghui commented on a diff in pull request #15603: [improve][broker] Make some methods in TenantsBase async.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #15603:
URL: https://github.com/apache/pulsar/pull/15603#discussion_r873289772
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java:
##########
@@ -63,24 +63,18 @@ public class TenantsBase extends PulsarWebResource {
@ApiResponses(value = {@ApiResponse(code = 403, message = "The requester doesn't have admin permissions"),
@ApiResponse(code = 404, message = "Tenant doesn't exist")})
public void getTenants(@Suspended final AsyncResponse asyncResponse) {
- final String clientAppId = clientAppId();
- try {
- validateSuperUserAccess();
- } catch (Exception e) {
- asyncResponse.resume(e);
- return;
- }
- tenantResources().listTenantsAsync().whenComplete((tenants, e) -> {
- if (e != null) {
- log.error("[{}] Failed to get tenants list", clientAppId, e);
- asyncResponse.resume(new RestException(e));
- return;
- }
- // deep copy the tenants to avoid concurrent sort exception
- List<String> deepCopy = new ArrayList<>(tenants);
- deepCopy.sort(null);
- asyncResponse.resume(deepCopy);
- });
+ validateSuperUserAccessAsync()
+ .thenCompose(__ -> tenantResources().listTenantsAsync())
+ .thenAccept(tenants -> {
+ // deep copy the tenants to avoid concurrent sort exception
+ List<String> deepCopy = new ArrayList<>(tenants);
+ deepCopy.sort(null);
+ asyncResponse.resume(deepCopy);
Review Comment:
```suggestion
asyncResponse.resume(Collections.unmodifiableListdeepCopy(deepCopy));
```
--
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 #15603: [improve][broker] Make some methods in TenantsBase async.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on code in PR #15603:
URL: https://github.com/apache/pulsar/pull/15603#discussion_r873090077
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java:
##########
@@ -90,23 +84,20 @@ public void getTenants(@Suspended final AsyncResponse asyncResponse) {
@ApiResponse(code = 404, message = "Tenant does not exist")})
public void getTenantAdmin(@Suspended final AsyncResponse asyncResponse,
@ApiParam(value = "The tenant name") @PathParam("tenant") String tenant) {
- final String clientAppId = clientAppId();
- try {
- validateSuperUserAccess();
- } catch (Exception e) {
- asyncResponse.resume(e);
- }
-
- tenantResources().getTenantAsync(tenant).whenComplete((tenantInfo, e) -> {
- if (e != null) {
- log.error("[{}] Failed to get Tenant {}", clientAppId, e.getMessage());
- asyncResponse.resume(new RestException(Status.INTERNAL_SERVER_ERROR, "Failed to get Tenant"));
- return;
- }
- boolean response = tenantInfo.isPresent() ? asyncResponse.resume(tenantInfo.get())
- : asyncResponse.resume(new RestException(Status.NOT_FOUND, "Tenant does not exist"));
- return;
- });
+ validateSuperUserAccessAsync()
+ .thenCompose(__ -> tenantResources().getTenantAsync(tenant))
+ .thenApply(tenantInfo -> {
+ if (!tenantInfo.isPresent()) {
+ new RestException(Status.NOT_FOUND, "Tenant does not exist");
+ }
+ return tenantInfo.get();
+ })
+ .thenAccept(asyncResponse::resume)
+ .exceptionally(ex -> {
+ log.error("[{}] Failed to get tenant admin {}", clientAppId(), ex);
Review Comment:
Keep the original behavour.
--
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] nodece commented on a diff in pull request #15603: [improve][broker] Make some methods in TenantsBase async.
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #15603:
URL: https://github.com/apache/pulsar/pull/15603#discussion_r873051574
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java:
##########
@@ -90,23 +84,20 @@ public void getTenants(@Suspended final AsyncResponse asyncResponse) {
@ApiResponse(code = 404, message = "Tenant does not exist")})
public void getTenantAdmin(@Suspended final AsyncResponse asyncResponse,
@ApiParam(value = "The tenant name") @PathParam("tenant") String tenant) {
- final String clientAppId = clientAppId();
- try {
- validateSuperUserAccess();
- } catch (Exception e) {
- asyncResponse.resume(e);
- }
-
- tenantResources().getTenantAsync(tenant).whenComplete((tenantInfo, e) -> {
- if (e != null) {
- log.error("[{}] Failed to get Tenant {}", clientAppId, e.getMessage());
- asyncResponse.resume(new RestException(Status.INTERNAL_SERVER_ERROR, "Failed to get Tenant"));
- return;
- }
- boolean response = tenantInfo.isPresent() ? asyncResponse.resume(tenantInfo.get())
- : asyncResponse.resume(new RestException(Status.NOT_FOUND, "Tenant does not exist"));
- return;
- });
+ validateSuperUserAccessAsync()
+ .thenCompose(__ -> tenantResources().getTenantAsync(tenant))
+ .thenApply(tenantInfo -> {
+ if (!tenantInfo.isPresent()) {
+ new RestException(Status.NOT_FOUND, "Tenant does not exist");
Review Comment:
miss `throw`.
--
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 #15603: [improve][broker] Make some methods in TenantsBase async.
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on code in PR #15603:
URL: https://github.com/apache/pulsar/pull/15603#discussion_r873300633
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java:
##########
@@ -63,24 +63,18 @@ public class TenantsBase extends PulsarWebResource {
@ApiResponses(value = {@ApiResponse(code = 403, message = "The requester doesn't have admin permissions"),
@ApiResponse(code = 404, message = "Tenant doesn't exist")})
public void getTenants(@Suspended final AsyncResponse asyncResponse) {
- final String clientAppId = clientAppId();
- try {
- validateSuperUserAccess();
- } catch (Exception e) {
- asyncResponse.resume(e);
- return;
- }
- tenantResources().listTenantsAsync().whenComplete((tenants, e) -> {
- if (e != null) {
- log.error("[{}] Failed to get tenants list", clientAppId, e);
- asyncResponse.resume(new RestException(e));
- return;
- }
- // deep copy the tenants to avoid concurrent sort exception
- List<String> deepCopy = new ArrayList<>(tenants);
- deepCopy.sort(null);
- asyncResponse.resume(deepCopy);
- });
+ validateSuperUserAccessAsync()
+ .thenCompose(__ -> tenantResources().listTenantsAsync())
+ .thenAccept(tenants -> {
+ // deep copy the tenants to avoid concurrent sort exception
+ List<String> deepCopy = new ArrayList<>(tenants);
+ deepCopy.sort(null);
+ asyncResponse.resume(deepCopy);
+ }).exceptionally(ex -> {
+ log.error("[{}] Failed to get tenants list", clientAppId(), ex);
Review Comment:
I'm not sure if the `HttpRequest` object will be reused in async way, if it reused the client appid may be modified.
--
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