fix get partition metadata problem for a non-existed topic by aloyszhang · Pull Request #8818 · apache/pulsar

aloyszhang · 2020-12-03T14:49:52Z

Motivation

Currently, getting the partition metadata for a non-existed topic, it returns 0 instead of throwing an exception.
This pr fix this by throwing an exception.

Modifications

If no metadata found in global zk, then will check whether the topic is exist, if yes, will return 0, otherwise will throw an exception.

BewareMyPower

A unit test should be added to cover the case when a PulsarAdmin tries to get the partition metadata of a non-existed topic.

BewareMyPower · 2020-12-04T01:33:04Z

Please rebase to the latest master since the master branch was broken before.

aloyszhang · 2020-12-04T06:41:57Z

@BewareMyPower I'll do this later.

wolfstudy · 2020-12-21T13:03:41Z

@BewareMyPower Please take a look this change again, thanks.

BewareMyPower · 2020-12-21T16:17:48Z

@aloyszhang could you rebase to latest master?

codelipenghui · 2020-12-24T13:25:18Z

+                if (e.getCause().getMessage().contains("Topic not exist.")) {
+                    throw new org.apache.pulsar.broker.web.RestException(Response.Status.NOT_FOUND, "Topic not exist.");
+                }


Could you please clarify why throw (RestException) e.getCause() does not work here? If e.getCause is instance of RestException, I think we can throw it directly. I might be missing something here.

I'll check this PR and push later.

Thanks @aloyszhang

codelipenghui · 2020-12-24T13:25:34Z

+                // Since CompletableFuture wrappers exception, so we will lost the Status
+                // rebuild the Status if exception message contains "Topic not exist."
+                if (e.getCause().getMessage().contains("Topic not exist.")) {
+                    throw new org.apache.pulsar.broker.web.RestException(Response.Status.NOT_FOUND, "Topic not exist.");
+                }


Same as the above comment.

codelipenghui · 2020-12-24T13:32:44Z

+            }
+        } catch (RestException restException) {
+            if (Status.NOT_FOUND.getStatusCode() != restException.getResponse().getStatus()) {
+                throw restException;


I think it's not correct handling here, If got 404 here, it means the partitioned metadata does not exist right? If the partitioned metadata does exist, why should we prevent the non-partitioned topic creation?

codelipenghui · 2020-12-24T13:35:36Z

+        CompletableFuture<List<String>> future = new CompletableFuture<>();
+        pulsar.getBrokerService().fetchPartitionedTopicMetadataAsync(topicName).whenComplete((meta, t) -> {
+            if (t != null) {
+                future.completeExceptionally(t);
+                return;
+            }
            List<String> result = Lists.newArrayList();
            for (int i = 0; i < meta.partitions; i++) {
                result.add(topicName.getPartition(i).toString());
            }
-            return CompletableFuture.completedFuture(result);
+            future.complete(result);
        });
+        return future;


What is the difference between these changes? Why we change the thenCompose to whenComplete?

eolivelli · 2020-12-25T22:57:41Z

            if (e.getCause() instanceof RestException) {
+                // Since CompletableFuture wrappers exception, so we will lost the Status
+                // rebuild the Status if exception message contains "Topic not exist."
+                if (e.getCause().getMessage().contains("Topic not exist.")) {


Please do not rely on strings, can we test the actual class?

Yes, I'll change this PR and push again.

codelipenghui · 2021-01-03T03:06:40Z

+                    pulsar().getBrokerService().fetchPartitionedTopicMetadataAsync(topicName).get();
+            if (partitionedTopicMetadata.partitions < 1) {
+                if (!pulsar().getNamespaceService().checkTopicExists(topicName).get()
+                        && checkAllowAutoCreation


Here is not covered by tests? I noticed the tests only cover the checkAllowAutoCreation=false, could you please help add a test for cover checkAllowAutoCreation=true?

I'll add test case to protect this part of code.

…tadata

eolivelli

LGTM

aloyszhang · 2021-01-05T03:14:16Z

@codelipenghui It seems merge apache/master branch into this pr branch makes more checks failed. For each test ,which namespaces is not explicit created, it'll failed now.

codelipenghui · 2021-01-06T05:45:10Z

/pulsarbot run-failure-checks

codelipenghui · 2021-01-06T14:25:30Z

/pulsarbot run-failure-checks

aloyszhang · 2021-01-06T16:07:48Z

/pulsarbot run-failure-checks

aloyszhang · 2021-01-06T17:36:51Z

/pulsarbot run-failure-checks

Fixes #8813 Currently, getting the partition metadata for a non-existed topic, it returns 0 instead of throwing an exception. This pr fix this by throwing an exception. If no metadata found in global zk, then will check whether the topic is exist, if yes, will return 0, otherwise will throw an exception. (cherry picked from commit a3dfb2a)

Fixes apache#8813 ### Motivation Currently, getting the partition metadata for a non-existed topic, it returns 0 instead of throwing an exception. This pr fix this by throwing an exception. ### Modifications If no metadata found in global zk, then will check whether the topic is exist, if yes, will return 0, otherwise will throw an exception.

BewareMyPower requested changes Dec 3, 2020

View reviewed changes

aloyszhang force-pushed the no-exist branch from f1a8cf6 to 78226ee Compare December 4, 2020 02:08

aloyszhang force-pushed the no-exist branch 3 times, most recently from 41b824b to 533e000 Compare December 7, 2020 16:14

sijie added triage/week-50 type/bug The PR fixed a bug or issue reported a bug labels Dec 8, 2020

sijie assigned aloyszhang Dec 8, 2020

sijie added this to the 2.8.0 milestone Dec 8, 2020

sijie added release/2.6.3 release/2.7.1 labels Dec 8, 2020

aloyszhang force-pushed the no-exist branch from 3facb47 to 88fd546 Compare December 16, 2020 14:50

BewareMyPower approved these changes Dec 21, 2020

View reviewed changes

aloyszhang force-pushed the no-exist branch from 88fd546 to 6e9ab2f Compare December 24, 2020 06:32

codelipenghui reviewed Dec 24, 2020

View reviewed changes

eolivelli requested changes Dec 25, 2020

View reviewed changes

codelipenghui removed the triage/week-50 label Dec 29, 2020

aloyszhang force-pushed the no-exist branch from 6e9ab2f to c619659 Compare January 1, 2021 10:43

codelipenghui reviewed Jan 3, 2021

View reviewed changes

aloyszhang force-pushed the no-exist branch from 6bd8ef5 to af52cf3 Compare January 4, 2021 06:59

add method to validate whether topic is exist when get partitioned me…

9af1bf3

…tadata

aloyszhang force-pushed the no-exist branch from af52cf3 to 9af1bf3 Compare January 4, 2021 07:09

aloyszhang added 2 commits January 4, 2021 15:20

revoke un-used change for test

1d82d97

add process for NotFoundException when unwrap throwable

c54c103

codelipenghui approved these changes Jan 4, 2021

View reviewed changes

eolivelli approved these changes Jan 4, 2021

View reviewed changes

Merge remote-tracking branch 'apache/master' into no-exist

72ff04d

codelipenghui added 2 commits January 6, 2021 07:58

Merge remote-tracking branch 'apache/master' into no-exist

133e1e3

Merge remote-tracking branch 'apache/master' into no-exist

242f097

fix test

6d5eeab

codelipenghui merged commit a3dfb2a into apache:master Jan 7, 2021

codelipenghui added the cherry-picked/branch-2.6 label Jan 7, 2021

codelipenghui added the cherry-picked/branch-2.7 Archived: 2.7 is end of life label Jan 7, 2021

aloyszhang deleted the no-exist branch January 22, 2021 11:58

BewareMyPower mentioned this pull request May 16, 2021

Fix getting partition metadata of a nonexistent topic returns 0 #10601

Merged

1 task

Uh oh!

Conversation

aloyszhang commented Dec 3, 2020

Motivation

Modifications

Uh oh!

BewareMyPower left a comment

Choose a reason for hiding this comment

Uh oh!

BewareMyPower commented Dec 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aloyszhang commented Dec 4, 2020

Uh oh!

wolfstudy commented Dec 21, 2020

Uh oh!

BewareMyPower commented Dec 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

aloyszhang commented Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codelipenghui commented Jan 6, 2021

Uh oh!

codelipenghui commented Jan 6, 2021

Uh oh!

aloyszhang commented Jan 6, 2021

Uh oh!

aloyszhang commented Jan 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

BewareMyPower commented Dec 4, 2020 •

edited

Loading

aloyszhang commented Jan 5, 2021 •

edited

Loading