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 2020/12/03 14:50:01 UTC
[GitHub] [pulsar] aloyszhang opened a new pull request #8818: fix get partition metadata problem for a non-existed topic
aloyszhang opened a new pull request #8818:
URL: https://github.com/apache/pulsar/pull/8818
Fixes #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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #8818: fix get partition metadata problem for a non-existed topic
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #8818:
URL: https://github.com/apache/pulsar/pull/8818#issuecomment-755094416
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a change in pull request #8818: fix get partition metadata problem for a non-existed topic
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on a change in pull request #8818:
URL: https://github.com/apache/pulsar/pull/8818#discussion_r548967935
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java
##########
@@ -702,6 +702,11 @@ protected static PartitionedTopicMetadata fetchPartitionedTopicMetadata(PulsarSe
return pulsar.getBrokerService().fetchPartitionedTopicMetadataAsync(topicName).get();
} catch (Exception e) {
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.")) {
Review comment:
Yes, I'll change this PR and push again.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #8818: fix get partition metadata problem for a non-existed topic
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #8818:
URL: https://github.com/apache/pulsar/pull/8818#issuecomment-755328306
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #8818: fix get partition metadata problem for a non-existed topic
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on pull request #8818:
URL: https://github.com/apache/pulsar/pull/8818#issuecomment-738599501
@BewareMyPower I'll do this later.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #8818: fix get partition metadata problem for a non-existed topic
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on pull request #8818:
URL: https://github.com/apache/pulsar/pull/8818#issuecomment-755391594
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #8818: fix get partition metadata problem for a non-existed topic
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #8818:
URL: https://github.com/apache/pulsar/pull/8818#discussion_r549183636
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java
##########
@@ -702,6 +702,11 @@ protected static PartitionedTopicMetadata fetchPartitionedTopicMetadata(PulsarSe
return pulsar.getBrokerService().fetchPartitionedTopicMetadataAsync(topicName).get();
} catch (Exception e) {
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.")) {
+ throw new org.apache.pulsar.broker.web.RestException(Response.Status.NOT_FOUND, "Topic not exist.");
+ }
Review comment:
Thanks @aloyszhang
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #8818: fix get partition metadata problem for a non-existed topic
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on pull request #8818:
URL: https://github.com/apache/pulsar/pull/8818#issuecomment-755389088
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang removed a comment on pull request #8818: fix get partition metadata problem for a non-existed topic
Posted by GitBox <gi...@apache.org>.
aloyszhang removed a comment on pull request #8818:
URL: https://github.com/apache/pulsar/pull/8818#issuecomment-755389088
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #8818: fix get partition metadata problem for a non-existed topic
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #8818:
URL: https://github.com/apache/pulsar/pull/8818#issuecomment-749056751
@aloyszhang could you rebase to latest master?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower edited a comment on pull request #8818: fix get partition metadata problem for a non-existed topic
Posted by GitBox <gi...@apache.org>.
BewareMyPower edited a comment on pull request #8818:
URL: https://github.com/apache/pulsar/pull/8818#issuecomment-738495026
Please rebase to the latest master since the master branch was broken before.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #8818: fix get partition metadata problem for a non-existed topic
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #8818:
URL: https://github.com/apache/pulsar/pull/8818#issuecomment-738495026
Please rebase to the latest master since the master was broken before,
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #8818: fix get partition metadata problem for a non-existed topic
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #8818:
URL: https://github.com/apache/pulsar/pull/8818
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang edited a comment on pull request #8818: fix get partition metadata problem for a non-existed topic
Posted by GitBox <gi...@apache.org>.
aloyszhang edited a comment on pull request #8818:
URL: https://github.com/apache/pulsar/pull/8818#issuecomment-754362147
@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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #8818: fix get partition metadata problem for a non-existed topic
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on pull request #8818:
URL: https://github.com/apache/pulsar/pull/8818#issuecomment-755448705
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #8818: fix get partition metadata problem for a non-existed topic
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #8818:
URL: https://github.com/apache/pulsar/pull/8818#discussion_r550948700
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java
##########
@@ -376,6 +377,25 @@ protected void validatePartitionedTopicMetadata(String tenant, String namespace,
}
}
+ protected void validateTopicExistedAndCheckAllowAutoCreation(String tenant, String namespace,
+ String encodedTopic, boolean checkAllowAutoCreation) {
+ try {
+ PartitionedTopicMetadata partitionedTopicMetadata =
+ pulsar().getBrokerService().fetchPartitionedTopicMetadataAsync(topicName).get();
+ if (partitionedTopicMetadata.partitions < 1) {
+ if (!pulsar().getNamespaceService().checkTopicExists(topicName).get()
+ && checkAllowAutoCreation
Review comment:
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`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #8818: fix get partition metadata problem for a non-existed topic
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #8818:
URL: https://github.com/apache/pulsar/pull/8818#discussion_r548529963
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java
##########
@@ -715,6 +720,11 @@ protected static PartitionedTopicMetadata fetchPartitionedTopicMetadataCheckAllo
.get();
} catch (Exception e) {
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.")) {
+ throw new org.apache.pulsar.broker.web.RestException(Response.Status.NOT_FOUND, "Topic not exist.");
+ }
Review comment:
Same as the above comment.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java
##########
@@ -702,6 +702,11 @@ protected static PartitionedTopicMetadata fetchPartitionedTopicMetadata(PulsarSe
return pulsar.getBrokerService().fetchPartitionedTopicMetadataAsync(topicName).get();
} catch (Exception e) {
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.")) {
+ throw new org.apache.pulsar.broker.web.RestException(Response.Status.NOT_FOUND, "Topic not exist.");
+ }
Review comment:
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.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -456,10 +456,17 @@ protected void internalCreateNonPartitionedTopic(boolean authoritative) {
validateTopicOwnership(topicName, authoritative);
- PartitionedTopicMetadata partitionMetadata = getPartitionedTopicMetadata(topicName, authoritative, false);
- if (partitionMetadata.partitions > 0) {
- log.warn("[{}] Partitioned topic with the same name already exists {}", clientAppId(), topicName);
- throw new RestException(Status.CONFLICT, "This topic already exists");
+ PartitionedTopicMetadata partitionMetadata = null;
+ try {
+ partitionMetadata = getPartitionedTopicMetadata(topicName, authoritative, false);
+ if (partitionMetadata.partitions > 0) {
+ log.warn("[{}] Partitioned topic with the same name already exists {}", clientAppId(), topicName);
+ throw new RestException(Status.CONFLICT, "This topic already exists");
+ }
+ } catch (RestException restException) {
+ if (Status.NOT_FOUND.getStatusCode() != restException.getResponse().getStatus()) {
+ throw restException;
Review comment:
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?
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java
##########
@@ -1174,13 +1174,19 @@ public ServiceUnitId getServiceUnitId(TopicName topicName) throws Exception {
}
private CompletableFuture<List<String>> getPartitionsForTopic(TopicName topicName) {
- return pulsar.getBrokerService().fetchPartitionedTopicMetadataAsync(topicName).thenCompose(meta -> {
+ 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;
Review comment:
What is the difference between these changes? Why we change the `thenCompose` to `whenComplete`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #8818: fix get partition metadata problem for a non-existed topic
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #8818:
URL: https://github.com/apache/pulsar/pull/8818#discussion_r551368003
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java
##########
@@ -376,6 +377,25 @@ protected void validatePartitionedTopicMetadata(String tenant, String namespace,
}
}
+ protected void validateTopicExistedAndCheckAllowAutoCreation(String tenant, String namespace,
+ String encodedTopic, boolean checkAllowAutoCreation) {
+ try {
+ PartitionedTopicMetadata partitionedTopicMetadata =
+ pulsar().getBrokerService().fetchPartitionedTopicMetadataAsync(topicName).get();
+ if (partitionedTopicMetadata.partitions < 1) {
+ if (!pulsar().getNamespaceService().checkTopicExists(topicName).get()
+ && checkAllowAutoCreation
Review comment:
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #8818: fix get partition metadata problem for a non-existed topic
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on pull request #8818:
URL: https://github.com/apache/pulsar/pull/8818#issuecomment-754362147
@codelipenghui It seems merge `apache/master` branch into this pr branch makes more checks failed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #8818: fix get partition metadata problem for a non-existed topic
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #8818:
URL: https://github.com/apache/pulsar/pull/8818#discussion_r548919714
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java
##########
@@ -702,6 +702,11 @@ protected static PartitionedTopicMetadata fetchPartitionedTopicMetadata(PulsarSe
return pulsar.getBrokerService().fetchPartitionedTopicMetadataAsync(topicName).get();
} catch (Exception e) {
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.")) {
Review comment:
Please do not rely on strings, can we test the actual class?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a change in pull request #8818: fix get partition metadata problem for a non-existed topic
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on a change in pull request #8818:
URL: https://github.com/apache/pulsar/pull/8818#discussion_r548823967
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java
##########
@@ -702,6 +702,11 @@ protected static PartitionedTopicMetadata fetchPartitionedTopicMetadata(PulsarSe
return pulsar.getBrokerService().fetchPartitionedTopicMetadataAsync(topicName).get();
} catch (Exception e) {
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.")) {
+ throw new org.apache.pulsar.broker.web.RestException(Response.Status.NOT_FOUND, "Topic not exist.");
+ }
Review comment:
I'll check this PR and push later.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a change in pull request #8818: fix get partition metadata problem for a non-existed topic
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on a change in pull request #8818:
URL: https://github.com/apache/pulsar/pull/8818#discussion_r551081111
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java
##########
@@ -376,6 +377,25 @@ protected void validatePartitionedTopicMetadata(String tenant, String namespace,
}
}
+ protected void validateTopicExistedAndCheckAllowAutoCreation(String tenant, String namespace,
+ String encodedTopic, boolean checkAllowAutoCreation) {
+ try {
+ PartitionedTopicMetadata partitionedTopicMetadata =
+ pulsar().getBrokerService().fetchPartitionedTopicMetadataAsync(topicName).get();
+ if (partitionedTopicMetadata.partitions < 1) {
+ if (!pulsar().getNamespaceService().checkTopicExists(topicName).get()
+ && checkAllowAutoCreation
Review comment:
I'll add test case to protect this part of code.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] wolfstudy commented on pull request #8818: fix get partition metadata problem for a non-existed topic
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on pull request #8818:
URL: https://github.com/apache/pulsar/pull/8818#issuecomment-748962810
@BewareMyPower Please take a look this change again, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org