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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
