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/02/14 10:24:05 UTC
[GitHub] [pulsar] Technoboy- opened a new pull request #14269: Fix validateGlobalNamespaceOwnership wrap exception issue.
Technoboy- opened a new pull request #14269:
URL: https://github.com/apache/pulsar/pull/14269
### Motivation
When Rest API call `AdminResource#validateGlobalNamespaceOwnership`, broker will execute `PulsarWebResource#checkLocalOrGetPeerReplicationCluster`.
In `PulsarWebResource#checkLocalOrGetPeerReplicationCluster`:
https://github.com/apache/pulsar/blob/6d717a08ef8cfcac032caee06105285594baf09f/pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java#L773-L802
Line 780, 794, and 801 has thrown RestException.
But `validateGlobalNamespaceOwnership ` has wrapped the exception :
https://github.com/apache/pulsar/blob/6d717a08ef8cfcac032caee06105285594baf09f/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java#L202-L216
This could make the user confused that the log printed is not matched with the REST API.
### Modification
- Add a relative test.
### 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] nodece commented on a change in pull request #14269: Fix validateGlobalNamespaceOwnership wrap exception issue.
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #14269:
URL: https://github.com/apache/pulsar/pull/14269#discussion_r806549727
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -796,9 +796,9 @@ protected void validateGlobalNamespaceOwnership(NamespaceName namespace) {
validationFuture.complete(null);
}
} else {
- String msg = String.format("Policies not found for %s namespace", namespace.toString());
+ String msg = String.format("Namespace %s not found", namespace.toString());
log.warn(msg);
- validationFuture.completeExceptionally(new RestException(Status.NOT_FOUND, msg));
+ validationFuture.completeExceptionally(new RestException(Status.NOT_FOUND, "Namespace not found"));
Review comment:
Sounds good, there is have a premise is that every namespace has policies.
--
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] aparajita89 commented on a change in pull request #14269: Fix validateGlobalNamespaceOwnership wrap exception issue.
Posted by GitBox <gi...@apache.org>.
aparajita89 commented on a change in pull request #14269:
URL: https://github.com/apache/pulsar/pull/14269#discussion_r806562481
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -796,9 +796,9 @@ protected void validateGlobalNamespaceOwnership(NamespaceName namespace) {
validationFuture.complete(null);
}
} else {
- String msg = String.format("Policies not found for %s namespace", namespace.toString());
+ String msg = String.format("Namespace %s not found", namespace.toString());
log.warn(msg);
- validationFuture.completeExceptionally(new RestException(Status.NOT_FOUND, msg));
+ validationFuture.completeExceptionally(new RestException(Status.NOT_FOUND, "Namespace not found"));
Review comment:
ah ok, just saw your comment @nodece
thanks
--
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] aparajita89 commented on a change in pull request #14269: Fix validateGlobalNamespaceOwnership wrap exception issue.
Posted by GitBox <gi...@apache.org>.
aparajita89 commented on a change in pull request #14269:
URL: https://github.com/apache/pulsar/pull/14269#discussion_r806559648
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -796,9 +796,9 @@ protected void validateGlobalNamespaceOwnership(NamespaceName namespace) {
validationFuture.complete(null);
}
} else {
- String msg = String.format("Policies not found for %s namespace", namespace.toString());
+ String msg = String.format("Namespace %s not found", namespace.toString());
log.warn(msg);
- validationFuture.completeExceptionally(new RestException(Status.NOT_FOUND, msg));
+ validationFuture.completeExceptionally(new RestException(Status.NOT_FOUND, "Namespace not found"));
Review comment:
i also have the same doubt as @nodece .
the if condition on line 773 is checking if the policies exist for the namespace. this means the namespace already exists but there are no policies. it seems like the original error message is more accurate. perhaps i am missing something?
--
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] aparajita89 commented on a change in pull request #14269: Fix validateGlobalNamespaceOwnership wrap exception issue.
Posted by GitBox <gi...@apache.org>.
aparajita89 commented on a change in pull request #14269:
URL: https://github.com/apache/pulsar/pull/14269#discussion_r806559648
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -796,9 +796,9 @@ protected void validateGlobalNamespaceOwnership(NamespaceName namespace) {
validationFuture.complete(null);
}
} else {
- String msg = String.format("Policies not found for %s namespace", namespace.toString());
+ String msg = String.format("Namespace %s not found", namespace.toString());
log.warn(msg);
- validationFuture.completeExceptionally(new RestException(Status.NOT_FOUND, msg));
+ validationFuture.completeExceptionally(new RestException(Status.NOT_FOUND, "Namespace not found"));
Review comment:
i also have the same doubt as @nodece .
the if condition on line 773 is checking if the policies exist for the namespace. this means the namespace already exists but there are no policies. it seems like the original error message is more accurate. perhaps i am missing something?
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -796,9 +796,9 @@ protected void validateGlobalNamespaceOwnership(NamespaceName namespace) {
validationFuture.complete(null);
}
} else {
- String msg = String.format("Policies not found for %s namespace", namespace.toString());
+ String msg = String.format("Namespace %s not found", namespace.toString());
log.warn(msg);
- validationFuture.completeExceptionally(new RestException(Status.NOT_FOUND, msg));
+ validationFuture.completeExceptionally(new RestException(Status.NOT_FOUND, "Namespace not found"));
Review comment:
ah ok, just saw your comment @nodece
thanks
--
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 #14269: Fix validateGlobalNamespaceOwnership wrap exception issue.
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #14269:
URL: https://github.com/apache/pulsar/pull/14269
--
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 change in pull request #14269: Fix validateGlobalNamespaceOwnership wrap exception issue.
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #14269:
URL: https://github.com/apache/pulsar/pull/14269#discussion_r806423027
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -796,9 +796,9 @@ protected void validateGlobalNamespaceOwnership(NamespaceName namespace) {
validationFuture.complete(null);
}
} else {
- String msg = String.format("Policies not found for %s namespace", namespace.toString());
+ String msg = String.format("Namespace %s not found", namespace.toString());
log.warn(msg);
- validationFuture.completeExceptionally(new RestException(Status.NOT_FOUND, msg));
+ validationFuture.completeExceptionally(new RestException(Status.NOT_FOUND, "Namespace not found"));
Review comment:
Why change this section? The old comment out looks great. If the policies is non-exist, we shouldn't throw the `Namespace %s not found`.
--
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] michaeljmarshall commented on pull request #14269: Fix validateGlobalNamespaceOwnership wrap exception issue.
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #14269:
URL: https://github.com/apache/pulsar/pull/14269#issuecomment-1049527923
I am unable to cherry-pick this to 2.8.3 because of test errors in the mocking. I am moving it to 2.8.4 for now. After cherry-picking and resolving conflicts, I run `mvn -pl :pulsar-broker test -Dtest="PersistentTopicsTest#testCreateTopicWithReplicationCluster"` and get the following test error:
```
[INFO] -------------------------------------------------------
[INFO] T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.apache.pulsar.broker.admin.PersistentTopicsTest
[ERROR] Tests run: 4, Failures: 1, Errors: 0, Skipped: 3, Time elapsed: 13.607 s <<< FAILURE! - in org.apache.pulsar.broker.admin.PersistentTopicsTest
[ERROR] testCreateTopicWithReplicationCluster(org.apache.pulsar.broker.admin.PersistentTopicsTest) Time elapsed: 0.007 s <<< FAILURE!
org.mockito.exceptions.misusing.WrongTypeOfReturnValue:
NamespaceResources$MockitoMock$634217304 cannot be returned by getPulsarResources()
getPulsarResources() should return PulsarResources
***
If you're unsure why you're getting above error read on.
Due to the nature of the syntax above problem might occur because:
1. This exception *might* occur in wrongly written multi-threaded tests.
Please refer to Mockito FAQ on limitations of concurrency testing.
2. A spy is stubbed using when(spy.foo()).then() syntax. It is safer to stub spies -
- with doReturn|Throw() family of methods. More in javadocs for Mockito.spy() method.
at org.apache.pulsar.broker.admin.PersistentTopicsTest.testCreateTopicWithReplicationCluster(PersistentTopicsTest.java:389)
at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132)
at org.testng.internal.InvokeMethodRunnable.runOne(InvokeMethodRunnable.java:45)
at org.testng.internal.InvokeMethodRunnable.call(InvokeMethodRunnable.java:73)
at org.testng.internal.InvokeMethodRunnable.call(InvokeMethodRunnable.java:11)
at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1130)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:630)
at java.base/java.lang.Thread.run(Thread.java:832)
[INFO]
[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR] org.apache.pulsar.broker.admin.PersistentTopicsTest.testCreateTopicWithReplicationCluster(org.apache.pulsar.broker.admin.PersistentTopicsTest)
[INFO] Run 1: PASS
[ERROR] Run 2: PersistentTopicsTest.testCreateTopicWithReplicationCluster:389 WrongTypeOfReturnValue
[INFO]
[INFO]
[ERROR] Tests run: 3, Failures: 1, Errors: 0, Skipped: 2
```
--
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 #14269: Fix validateGlobalNamespaceOwnership wrap exception issue.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14269:
URL: https://github.com/apache/pulsar/pull/14269#discussion_r806461113
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -796,9 +796,9 @@ protected void validateGlobalNamespaceOwnership(NamespaceName namespace) {
validationFuture.complete(null);
}
} else {
- String msg = String.format("Policies not found for %s namespace", namespace.toString());
+ String msg = String.format("Namespace %s not found", namespace.toString());
log.warn(msg);
- validationFuture.completeExceptionally(new RestException(Status.NOT_FOUND, msg));
+ validationFuture.completeExceptionally(new RestException(Status.NOT_FOUND, "Namespace not found"));
Review comment:
Not looks great. Because policies mean namespace. If there is a namespace, there are policies. For user, it's about `namespace` here.
And some logs in Namespaces methods are also the same, not `policies` but `namespace`. Please help confirm.
--
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 change in pull request #14269: Fix validateGlobalNamespaceOwnership wrap exception issue.
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #14269:
URL: https://github.com/apache/pulsar/pull/14269#discussion_r806549727
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -796,9 +796,9 @@ protected void validateGlobalNamespaceOwnership(NamespaceName namespace) {
validationFuture.complete(null);
}
} else {
- String msg = String.format("Policies not found for %s namespace", namespace.toString());
+ String msg = String.format("Namespace %s not found", namespace.toString());
log.warn(msg);
- validationFuture.completeExceptionally(new RestException(Status.NOT_FOUND, msg));
+ validationFuture.completeExceptionally(new RestException(Status.NOT_FOUND, "Namespace not found"));
Review comment:
Sound good, there is have a premise is that every namespace has policies.
--
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 #14269: Fix validateGlobalNamespaceOwnership wrap exception issue.
Posted by GitBox <gi...@apache.org>.
Technoboy- commented on a change in pull request #14269:
URL: https://github.com/apache/pulsar/pull/14269#discussion_r806461113
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -796,9 +796,9 @@ protected void validateGlobalNamespaceOwnership(NamespaceName namespace) {
validationFuture.complete(null);
}
} else {
- String msg = String.format("Policies not found for %s namespace", namespace.toString());
+ String msg = String.format("Namespace %s not found", namespace.toString());
log.warn(msg);
- validationFuture.completeExceptionally(new RestException(Status.NOT_FOUND, msg));
+ validationFuture.completeExceptionally(new RestException(Status.NOT_FOUND, "Namespace not found"));
Review comment:
Not looks great. Because policies mean namespace. If there is a namespace, there are policies. For user, it's about `namespace` here.
And some logs in Namespaces methods are also the same, not `policies` but `namespace`. Please help confirm.
--
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 #14269: Fix validateGlobalNamespaceOwnership wrap exception issue.
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #14269:
URL: https://github.com/apache/pulsar/pull/14269
--
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 change in pull request #14269: Fix validateGlobalNamespaceOwnership wrap exception issue.
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #14269:
URL: https://github.com/apache/pulsar/pull/14269#discussion_r806423027
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -796,9 +796,9 @@ protected void validateGlobalNamespaceOwnership(NamespaceName namespace) {
validationFuture.complete(null);
}
} else {
- String msg = String.format("Policies not found for %s namespace", namespace.toString());
+ String msg = String.format("Namespace %s not found", namespace.toString());
log.warn(msg);
- validationFuture.completeExceptionally(new RestException(Status.NOT_FOUND, msg));
+ validationFuture.completeExceptionally(new RestException(Status.NOT_FOUND, "Namespace not found"));
Review comment:
Why change this section? The old comment out looks great. If the policies is non-exist, we shouldn't throw the `Namespace %s not found`.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -796,9 +796,9 @@ protected void validateGlobalNamespaceOwnership(NamespaceName namespace) {
validationFuture.complete(null);
}
} else {
- String msg = String.format("Policies not found for %s namespace", namespace.toString());
+ String msg = String.format("Namespace %s not found", namespace.toString());
log.warn(msg);
- validationFuture.completeExceptionally(new RestException(Status.NOT_FOUND, msg));
+ validationFuture.completeExceptionally(new RestException(Status.NOT_FOUND, "Namespace not found"));
Review comment:
Sound good, there is have a premise is that every namespace has policies.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java
##########
@@ -796,9 +796,9 @@ protected void validateGlobalNamespaceOwnership(NamespaceName namespace) {
validationFuture.complete(null);
}
} else {
- String msg = String.format("Policies not found for %s namespace", namespace.toString());
+ String msg = String.format("Namespace %s not found", namespace.toString());
log.warn(msg);
- validationFuture.completeExceptionally(new RestException(Status.NOT_FOUND, msg));
+ validationFuture.completeExceptionally(new RestException(Status.NOT_FOUND, "Namespace not found"));
Review comment:
Sounds good, there is have a premise is that every namespace has policies.
--
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