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/03/17 08:10:56 UTC
[GitHub] [pulsar] Technoboy- opened a new pull request #14730: Add namespace state check.
Technoboy- opened a new pull request #14730:
URL: https://github.com/apache/pulsar/pull/14730
Fixes #10263
Master Issue: #10263
### Motivation
When deleting namespace, the namespace policy is set to `delete=true`, but we don't check this state anymore. We should check this state at the check-policy stage.
After adding this check, we could resolve #10263.
Related email discuss here: https://lists.apache.org/thread/p4tmgk23l8j554bjr4h4ofk97bll5gp4
### Documentation
- [x] `no-need-doc`
--
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] Jason918 commented on a change in pull request #14730: [fix][broker]: Fix cannot delete namespace with system topic
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #14730:
URL: https://github.com/apache/pulsar/pull/14730#discussion_r831733540
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -791,6 +791,11 @@ protected void validateGlobalNamespaceOwnership(NamespaceName namespace) {
log.warn(msg);
validationFuture.completeExceptionally(new RestException(Status.PRECONDITION_FAILED, msg));
+ } else if (policies.deleted) {
Review comment:
@Technoboy-
My concern is more about if we delete a namespace, we may still get previous error info "Namespace does not have any clusters configured : local_cluster=%s ns=%s".
It's confusing for users.
> so it should be checked later to reduce the judgment.
The performance issue here is insignificant.
--
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 change in pull request #14730: [fix][broker]: Fix cannot delete namespace with system topic
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14730:
URL: https://github.com/apache/pulsar/pull/14730#discussion_r830754929
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -791,6 +791,11 @@ protected void validateGlobalNamespaceOwnership(NamespaceName namespace) {
log.warn(msg);
validationFuture.completeExceptionally(new RestException(Status.PRECONDITION_FAILED, msg));
+ } else if (policies.deleted) {
+ String msg = String.format("Namespace %s is deleted", namespace.toString());
+ log.warn(msg);
+ validationFuture.completeExceptionally(new RestException(Status.PRECONDITION_FAILED,
+ "Namespace is deleted"));
Review comment:
Ah, this is the same with line- 805. That means no detailed namespace info in the response status text.
--
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 change in pull request #14730: [fix][broker]: Fix cannot delete namespace with system topic
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14730:
URL: https://github.com/apache/pulsar/pull/14730#discussion_r831103587
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -791,6 +791,11 @@ protected void validateGlobalNamespaceOwnership(NamespaceName namespace) {
log.warn(msg);
validationFuture.completeExceptionally(new RestException(Status.PRECONDITION_FAILED, msg));
+ } else if (policies.deleted) {
Review comment:
Hi, I think we should check this `deleted` mark at the last, because namespace deletion is a low-frequency operation, so it should be checked later to reduce the judgment. What do you think?
--
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- removed a comment on pull request #14730: [fix][broker]: Fix cannot delete namespace with system topic
Posted by GitBox <gi...@apache.org>.
Technoboy- removed a comment on pull request #14730:
URL: https://github.com/apache/pulsar/pull/14730#issuecomment-1073447297
@Jason918 @yuruguo @wolfstudy Could you please help review ?
--
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 change in pull request #14730: [fix][broker]: Fix cannot delete namespace with system topic
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14730:
URL: https://github.com/apache/pulsar/pull/14730#discussion_r831780114
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -791,6 +791,11 @@ protected void validateGlobalNamespaceOwnership(NamespaceName namespace) {
log.warn(msg);
validationFuture.completeExceptionally(new RestException(Status.PRECONDITION_FAILED, msg));
+ } else if (policies.deleted) {
Review comment:
> @Technoboy- My concern is more about if we delete a namespace, we may still get previous error info "Namespace does not have any clusters configured : local_cluster=%s ns=%s". It's confusing for users.
>
> > so it should be checked later to reduce the judgment.
>
> The performance issue here is insignificant.
yes, agree.
--
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 change in pull request #14730: [fix][broker]: Fix cannot delete namespace with system topic
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14730:
URL: https://github.com/apache/pulsar/pull/14730#discussion_r831781274
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -791,6 +791,11 @@ protected void validateGlobalNamespaceOwnership(NamespaceName namespace) {
log.warn(msg);
validationFuture.completeExceptionally(new RestException(Status.PRECONDITION_FAILED, msg));
+ } else if (policies.deleted) {
Review comment:
fixed, thanks @Jason918 @codelipenghui @shibd .
--
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 #14730: [fix][broker]: Fix cannot delete namespace with system topic
Posted by GitBox <gi...@apache.org>.
Technoboy- merged pull request #14730:
URL: https://github.com/apache/pulsar/pull/14730
--
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] Jason918 commented on a change in pull request #14730: [fix][broker]: Fix cannot delete namespace with system topic
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #14730:
URL: https://github.com/apache/pulsar/pull/14730#discussion_r830972013
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -791,6 +791,11 @@ protected void validateGlobalNamespaceOwnership(NamespaceName namespace) {
log.warn(msg);
validationFuture.completeExceptionally(new RestException(Status.PRECONDITION_FAILED, msg));
+ } else if (policies.deleted) {
Review comment:
Should we check this `deleted` mark first, then the rest conditions in L774 and L780?
--
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 pull request #14730: [fix][broker]: Fix cannot delete namespace with system topic
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on pull request #14730:
URL: https://github.com/apache/pulsar/pull/14730#issuecomment-1073447266
--
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] Demogorgon314 commented on a change in pull request #14730: [fix][broker]: Fix cannot delete namespace with system topic
Posted by GitBox <gi...@apache.org>.
Demogorgon314 commented on a change in pull request #14730:
URL: https://github.com/apache/pulsar/pull/14730#discussion_r830739585
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -791,6 +791,11 @@ protected void validateGlobalNamespaceOwnership(NamespaceName namespace) {
log.warn(msg);
validationFuture.completeExceptionally(new RestException(Status.PRECONDITION_FAILED, msg));
+ } else if (policies.deleted) {
+ String msg = String.format("Namespace %s is deleted", namespace.toString());
+ log.warn(msg);
+ validationFuture.completeExceptionally(new RestException(Status.PRECONDITION_FAILED,
+ "Namespace is deleted"));
Review comment:
```suggestion
msg));
```
--
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 pull request #14730: [fix][broker]: Fix cannot delete namespace with system topic
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on pull request #14730:
URL: https://github.com/apache/pulsar/pull/14730#issuecomment-1073147481
Hi @michaeljmarshall @shibd @dave2wave @RobertIndie Could you help review this ?
--
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 change in pull request #14730: [fix][broker]: Fix cannot delete namespace with system topic
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #14730:
URL: https://github.com/apache/pulsar/pull/14730#discussion_r831750436
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -791,6 +791,11 @@ protected void validateGlobalNamespaceOwnership(NamespaceName namespace) {
log.warn(msg);
validationFuture.completeExceptionally(new RestException(Status.PRECONDITION_FAILED, msg));
+ } else if (policies.deleted) {
Review comment:
+1
Check the namespace delete or not in advance is more reasonable here, after users deleted a namespace, they should not get errors like "Namespace does not have any clusters configured".
--
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