[GitHub] [pulsar] mattisonchao commented on a diff in pull request #14153: [Broker] Make PersistentTopicsBase#internalGetPartitionedMetadata async

[GitBox <gi...@apache.org>]

mattisonchao commented on code in PR #14153:
URL: https://github.com/apache/pulsar/pull/14153#discussion_r865592644


##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/v1/V1_AdminApi2Test.java:
##########
@@ -256,7 +256,11 @@ public void nonPersistentTopics() throws Exception {
 
         // test partitioned-topic
         final String partitionedTopicName = "non-persistent://prop-xyz/use/ns1/paritioned";
-        assertEquals(admin.nonPersistentTopics().getPartitionedTopicMetadata(partitionedTopicName).partitions, 0);
+        try {
+            admin.nonPersistentTopics().getPartitionedTopicMetadata(partitionedTopicName);

Review Comment:
   missing `fail()` here?



##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java:
##########
@@ -546,27 +548,46 @@ private CompletableFuture<Void> updatePartitionInOtherCluster(int numPartitions,
 
     protected PartitionedTopicMetadata internalGetPartitionedMetadata(boolean authoritative,
                                                                       boolean checkAllowAutoCreation) {
-        PartitionedTopicMetadata metadata = getPartitionedTopicMetadata(topicName,
-                authoritative, checkAllowAutoCreation);
-        if (metadata.partitions == 0 && !checkAllowAutoCreation) {
-            // The topic may be a non-partitioned topic, so check if it exists here.
-            // However, when checkAllowAutoCreation is true, the client will create the topic if it doesn't exist.
-            // In this case, `partitions == 0` means the automatically created topic is a non-partitioned topic so we
-            // shouldn't check if the topic exists.
-            try {
-                if (!pulsar().getNamespaceService().checkTopicExists(topicName).get()) {
-                    throw new RestException(Status.NOT_FOUND,
-                            new PulsarClientException.NotFoundException("Topic not exist"));
-                }
-            } catch (InterruptedException | ExecutionException e) {
-                log.error("Failed to check if topic '{}' exists", topicName, e);
-                throw new RestException(Status.INTERNAL_SERVER_ERROR, "Failed to get topic metadata");
+        try {
+            return internalGetPartitionedMetadataAsync(authoritative, checkAllowAutoCreation)

Review Comment:
   Maybe we can use `PulsarWebResource#sync` to reduce duplicate code.



##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java:
##########
@@ -4135,15 +4156,15 @@ private CompletableFuture<Void> createSubscriptions(TopicName topicName, int num
     // Pulsar client-java lib always passes user-agent as X-Java-$version.
     // However, cpp-client older than v1.20 (PR #765) never used to pass it.
     // So, request without user-agent and Pulsar-CPP-vX (X < 1.21) must be rejected
-    private void validateClientVersion() {
+    protected CompletableFuture<Void> internalValidateClientVersionAsync() {

Review Comment:
   Question: 
   Looks like we don't have the async operation in this method.



##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java:
##########
@@ -423,21 +426,29 @@ protected void setServletContext(ServletContext servletContext) {
         this.servletContext = servletContext;
     }
 
-    protected CompletableFuture<PartitionedTopicMetadata> getPartitionedTopicMetadataAsync(
-            TopicName topicName, boolean authoritative, boolean checkAllowAutoCreation) {
+    protected PartitionedTopicMetadata getPartitionedTopicMetadata(TopicName topicName,
+                                                                   boolean authoritative,
+                                                                   boolean checkAllowAutoCreation) {
         try {
-            validateClusterOwnership(topicName.getCluster());
-        } catch (Exception e) {
-            return FutureUtil.failedFuture(e);
+            return getPartitionedTopicMetadataAsync(topicName, authoritative, checkAllowAutoCreation)

Review Comment:
   Maybe we can use `PulsarWebResource#sync` to reduce duplicate code.



##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java:
##########
@@ -4154,18 +4175,18 @@ private void validateClientVersion() {
                 if (splits != null && splits.length > 1) {
                     if (LEAST_SUPPORTED_CLIENT_VERSION_PREFIX.getMajorVersion() > Integer.parseInt(splits[0])
                             || LEAST_SUPPORTED_CLIENT_VERSION_PREFIX.getMinorVersion() > Integer.parseInt(splits[1])) {
-                        throw new RestException(Status.METHOD_NOT_ALLOWED,
+                        return FutureUtil.failedFuture(new RestException(Status.METHOD_NOT_ALLOWED,
                                 "Client lib is not compatible to access partitioned metadata: version " + userAgent
-                                        + " is not supported");
+                                        + " is not supported"));
                     }
                 }
             } catch (RestException re) {
-                throw re;
+                return FutureUtil.failedFuture(re);
             } catch (Exception e) {
                 log.warn("[{}] Failed to parse version {} ", clientAppId(), userAgent);

Review Comment:
   Question:
   I saw it existed in the previous code, why do we ignore this exception?



-- 
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