Fix validateGlobalNamespaceOwnership wrap exception issue.

[Technoboy-]

Motivation

When Rest API call AdminResource#validateGlobalNamespaceOwnership, broker will execute PulsarWebResource#checkLocalOrGetPeerReplicationCluster.
In PulsarWebResource#checkLocalOrGetPeerReplicationCluster:

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/web/PulsarWebResource.java

Lines 773 to 802 in 6d717a0

	if (policiesResult.isPresent()) {
	Policies policies = policiesResult.get();
	if (policies.replication_clusters.isEmpty()) {
	String msg = String.format(
	"Namespace does not have any clusters configured : local_cluster=%s ns=%s",
	localCluster, namespace.toString());
	log.warn(msg);
	validationFuture.completeExceptionally(new RestException(Status.PRECONDITION_FAILED, msg));
	} else if (!policies.replication_clusters.contains(localCluster)) {
	ClusterDataImpl ownerPeerCluster = getOwnerFromPeerClusterList(pulsarService,
	policies.replication_clusters);
	if (ownerPeerCluster != null) {
	// found a peer that own this namespace
	validationFuture.complete(ownerPeerCluster);
	return;
	}
	String msg = String.format(
	"Namespace missing local cluster name in clusters list: local_cluster=%s ns=%s clusters=%s",
	localCluster, namespace.toString(), policies.replication_clusters);
	log.warn(msg);
	validationFuture.completeExceptionally(new RestException(Status.PRECONDITION_FAILED, msg));
	} else {
	validationFuture.complete(null);
	}
	} else {
	String msg = String.format("Policies not found for %s namespace", namespace.toString());
	log.warn(msg);
	validationFuture.completeExceptionally(new RestException(Status.NOT_FOUND, msg));
	}

Line 780, 794, and 801 has thrown RestException.
But validateGlobalNamespaceOwnership has wrapped the exception :

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java

Lines 202 to 216 in 6d717a0

	protected void validateGlobalNamespaceOwnership() {
	try {
	validateGlobalNamespaceOwnership(this.namespaceName);
	} catch (IllegalArgumentException e) {
	throw new RestException(Status.PRECONDITION_FAILED, "Tenant name or namespace is not valid");
	} catch (RestException re) {
	if (re.getResponse().getStatus() == Status.NOT_FOUND.getStatusCode()) {
	throw new RestException(Status.NOT_FOUND, "Namespace not found");
	}
	throw new RestException(Status.PRECONDITION_FAILED, "Namespace does not have any clusters configured");
	} catch (Exception e) {
	log.warn("Failed to validate global cluster configuration : ns={} emsg={}", namespaceName, e.getMessage());
	throw new RestException(Status.SERVICE_UNAVAILABLE, "Failed to validate global cluster configuration");
	}
	}

This could make the user confused that the log printed is not matched with the REST API.

Modification

• Add a relative test.

Documentation

• no-need-doc

• doc-not-needed

[nodece]

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.

[Technoboy-]

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.

[nodece]

Sounds good, there is have a premise is that every namespace has policies.

[aparajita89]

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?

[aparajita89]

ah ok, just saw your comment @nodece
thanks

[nodece]

LGTM

[michaeljmarshall]

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