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/06/04 17:52:47 UTC
[GitHub] [pulsar] poorbarcode opened a new pull request, #15932: [fix] [admin] fix no matching code if the tenant already exists
poorbarcode opened a new pull request, #15932:
URL: https://github.com/apache/pulsar/pull/15932
### Motivation
When I use the Admin API to create a tenant, if the tenants are limited and I already have the max # of tenants, I get an error "412 Reached the max tenants [n] restriction." This is good, but if I'm trying to create a tenant that already exists, I get the same error. But, creating a tenant that already exists should be a no-op and would not actually push me over my limit.
"409 Already exists" is a softer error than "412 Reached the max restriction." It might be a little cleaner if Pulsar checked if the tenant exists first, and returned that error, before throwing the 412 exception. Otherwise the responsibility to fetch the tenants and check if it already exists must be done by the user.
*Explain here the context, and why you're making that change. What is the problem you're trying to solve.*
### Modifications
Swap the execution order of duplicate validation and maximum validation
### Documentation
- [ ] `doc-required`
- [x] `doc-not-needed`
- [ ] `doc`
- [ ] `doc-complete`
--
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] poorbarcode commented on pull request #15932: [fix] [admin] fix reach max tenants error if the tenant already exists
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #15932:
URL: https://github.com/apache/pulsar/pull/15932#issuecomment-1148611840
> @poorbarcode Could you please help create separate PR to fix branch-2.9 and branch-2.10? Due to the REST API async change, there are many conflicts when cherry-picking this PR.
OK I have submit a PR #15961
--
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] merlimat commented on a diff in pull request #15932: [fix] [admin] fix no matching code if the tenant already exists
Posted by GitBox <gi...@apache.org>.
merlimat commented on code in PR #15932:
URL: https://github.com/apache/pulsar/pull/15932#discussion_r889600730
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java:
##########
@@ -124,6 +124,12 @@ public void createTenant(@Suspended final AsyncResponse asyncResponse,
validateSuperUserAccessAsync()
.thenCompose(__ -> validatePoliciesReadOnlyAccessAsync())
.thenCompose(__ -> validateClustersAsync(tenantInfo))
+ .thenCompose(__ -> tenantResources().tenantExistsAsync(tenant))
+ .thenAccept(exist -> {
+ if (exist) {
+ throw new RestException(Status.CONFLICT, "Tenant already exist");
+ }
+ })
Review Comment:
We should also add a unit test to ensure the exception is the expected one (and that later on, the behavior will not be broken).
--
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 merged pull request #15932: [fix] [admin] fix reach max tenants error if the tenant already exists
Posted by GitBox <gi...@apache.org>.
codelipenghui merged PR #15932:
URL: https://github.com/apache/pulsar/pull/15932
--
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 pull request #15932: [fix] [admin] fix reach max tenants error if the tenant already exists
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on PR #15932:
URL: https://github.com/apache/pulsar/pull/15932#issuecomment-1148173355
@poorbarcode Could you please help create separate PR to fix branch-2.9 and branch-2.10? Due to the REST API async change, there are many conflicts when cherry-picking this PR.
--
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] poorbarcode commented on a diff in pull request #15932: [fix] [admin] fix no matching code if the tenant already exists
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #15932:
URL: https://github.com/apache/pulsar/pull/15932#discussion_r889605124
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/TenantsBase.java:
##########
@@ -124,6 +124,12 @@ public void createTenant(@Suspended final AsyncResponse asyncResponse,
validateSuperUserAccessAsync()
.thenCompose(__ -> validatePoliciesReadOnlyAccessAsync())
.thenCompose(__ -> validateClustersAsync(tenantInfo))
+ .thenCompose(__ -> tenantResources().tenantExistsAsync(tenant))
+ .thenAccept(exist -> {
+ if (exist) {
+ throw new RestException(Status.CONFLICT, "Tenant already exist");
+ }
+ })
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