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 2022/02/09 02:39:29 UTC
[GitHub] [pulsar] liudezhi2098 opened a new pull request #14179: [Broker]make getPermissionsOnTopic method async
liudezhi2098 opened a new pull request #14179:
URL: https://github.com/apache/pulsar/pull/14179
Master Issue: #14013
### Motivation
Avoid call sync method in async rest API for PersistentTopicsBase#internalGetPermissionsOnTopic.
### Modifications
- *Use async instead of sync method.*
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): (no)
- The public API: (no)
- The schema: (no)
- The default values of configurations: (no)
- The wire protocol: (no)
- The rest endpoints: (no)
- The admin cli options: (no)
- Anything that affects deployment: (no)
### Documentation
Need to update docs?
- [x] `no-need-doc`
--
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
[GitHub] [pulsar] mattisonchao commented on a change in pull request #14179: [Broker]make getPermissionsOnTopic method async
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on a change in pull request #14179:
URL: https://github.com/apache/pulsar/pull/14179#discussion_r803203384
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java
##########
@@ -138,15 +139,26 @@ public void getList(
@ApiResponse(code = 404, message = "tenant/namespace/topic doesn't exit"),
@ApiResponse(code = 412, message = "Topic name is not valid"),
@ApiResponse(code = 500, message = "Internal server error")})
- public Map<String, Set<AuthAction>> getPermissionsOnTopic(
+ public void getPermissionsOnTopic(
+ @Suspended final AsyncResponse asyncResponse,
@ApiParam(value = "Specify the tenant", required = true)
@PathParam("tenant") String tenant,
@ApiParam(value = "Specify the namespace", required = true)
@PathParam("namespace") String namespace,
@ApiParam(value = "Specify topic name", required = true)
@PathParam("topic") @Encoded String encodedTopic) {
- validateTopicName(tenant, namespace, encodedTopic);
- return internalGetPermissionsOnTopic();
+ try {
+ validateTopicName(tenant, namespace, encodedTopic);
+ internalGetPermissionsOnTopic().thenAccept(permissions -> asyncResponse.resume(permissions))
+ .exceptionally(ex -> {
+ Throwable realCause = FutureUtil.unwrapCompletionException(ex);
Review comment:
Same above.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v1/PersistentTopics.java
##########
@@ -105,11 +105,23 @@ public void getList(@Suspended final AsyncResponse asyncResponse, @PathParam("pr
+ "namespace level combined (union) with any eventual specific permission set on the topic.")
@ApiResponses(value = {@ApiResponse(code = 403, message = "Don't have admin permission"),
@ApiResponse(code = 404, message = "Namespace doesn't exist")})
- public Map<String, Set<AuthAction>> getPermissionsOnTopic(@PathParam("property") String property,
- @PathParam("cluster") String cluster, @PathParam("namespace") String namespace,
- @PathParam("topic") @Encoded String encodedTopic) {
- validateTopicName(property, cluster, namespace, encodedTopic);
- return internalGetPermissionsOnTopic();
+ public void getPermissionsOnTopic(@Suspended AsyncResponse asyncResponse,
+ @PathParam("property") String property,
+ @PathParam("cluster") String cluster,
+ @PathParam("namespace") String namespace,
+ @PathParam("topic") @Encoded String encodedTopic) {
+ try {
+ validateTopicName(property, cluster, namespace, encodedTopic);
+ internalGetPermissionsOnTopic().thenAccept(permissions -> asyncResponse.resume(permissions))
+ .exceptionally(ex -> {
+ Throwable realCause = FutureUtil.unwrapCompletionException(ex);
+ log.error("[{}] Failed to get permissions for topic {}", clientAppId(), topicName, ex);
+ return handleCommonRestAsyncException(asyncResponse, realCause);
Review comment:
You can use ``resumeAsyncResponseExceptionally`` to instead of ``handleCommonRestAsyncException``. because ``handleCommonRestAsyncException`` will be removed.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v1/PersistentTopics.java
##########
@@ -105,11 +105,23 @@ public void getList(@Suspended final AsyncResponse asyncResponse, @PathParam("pr
+ "namespace level combined (union) with any eventual specific permission set on the topic.")
@ApiResponses(value = {@ApiResponse(code = 403, message = "Don't have admin permission"),
@ApiResponse(code = 404, message = "Namespace doesn't exist")})
- public Map<String, Set<AuthAction>> getPermissionsOnTopic(@PathParam("property") String property,
- @PathParam("cluster") String cluster, @PathParam("namespace") String namespace,
- @PathParam("topic") @Encoded String encodedTopic) {
- validateTopicName(property, cluster, namespace, encodedTopic);
- return internalGetPermissionsOnTopic();
+ public void getPermissionsOnTopic(@Suspended AsyncResponse asyncResponse,
+ @PathParam("property") String property,
+ @PathParam("cluster") String cluster,
+ @PathParam("namespace") String namespace,
+ @PathParam("topic") @Encoded String encodedTopic) {
+ try {
+ validateTopicName(property, cluster, namespace, encodedTopic);
+ internalGetPermissionsOnTopic().thenAccept(permissions -> asyncResponse.resume(permissions))
+ .exceptionally(ex -> {
+ Throwable realCause = FutureUtil.unwrapCompletionException(ex);
Review comment:
If you use ``handleCommonRestAsyncException ``, you don't need to unwrap the exception, because ``handleCommonRestAsyncException`` already does that.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -193,44 +193,39 @@
return getPartitionedTopicList(TopicDomain.getEnum(domain()));
}
- protected Map<String, Set<AuthAction>> internalGetPermissionsOnTopic() {
+ protected CompletableFuture<Map<String, Set<AuthAction>>> internalGetPermissionsOnTopic() {
// This operation should be reading from zookeeper and it should be allowed without having admin privileges
validateAdminAccessForTenant(namespaceName.getTenant());
Review comment:
This method can be async.
--
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
[GitHub] [pulsar] github-actions[bot] commented on pull request #14179: [Broker]make getPermissionsOnTopic method async
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14179:
URL: https://github.com/apache/pulsar/pull/14179#issuecomment-1065784970
The pr had no activity for 30 days, mark with Stale label.
--
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