-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Broker] Make PersistentTopicsBase#internalDeleteTopic async #14069
Conversation
Motivation See apache#14013 Modification * Make PersistentTopicsBase#internalDeleteTopic async Signed-off-by: Zike Yang <[email protected]>
LGTM |
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
Outdated
Show resolved
Hide resolved
Could we try to merge these two delete methods? |
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java
Show resolved
Hide resolved
response = mock(AsyncResponse.class); | ||
persistentTopics.deleteTopic(response, testTenant, testNamespace, topicName, true, true, true); | ||
verify(response, timeout(5000).times(1)).resume(errorCaptor.capture()); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the errorCaptor.getValue()
: has exception and instanceof RestException
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errorCaptor
already checks that instance type.
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/PersistentTopicsTest.java
Show resolved
Hide resolved
Signed-off-by: Zike Yang <[email protected]>
Signed-off-by: Zike Yang <[email protected]>
@PathParam("namespace") String namespace, @PathParam("topic") @Encoded String encodedTopic, | ||
@QueryParam("force") @DefaultValue("false") boolean force, | ||
@QueryParam("authoritative") @DefaultValue("false") boolean authoritative, | ||
@QueryParam("deleteSchema") @DefaultValue("false") boolean deleteSchema) { | ||
validateTopicName(property, cluster, namespace, encodedTopic); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need catch exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as #14064 (comment)
The pr had no activity for 30 days, mark with Stale label. |
The pr had no activity for 30 days, mark with Stale label. |
These changes have been moved to #16232. So close this PR. |
Master Issue: #14013
Motivation
See #14013
Modifications
Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Documentation
no-need-doc