Hi Xiaoyu Hou, Thank you for your proposal. I agree that this proposal could help reduce unnecessary overhead by calling the internal broker's methods directly instead of initiating a new connection, with all that entails. It might be interesting to hear why it was implemented that way in the first place.
As you mentioned, there are other internal calls. I see additional usages of the `pulsar().getAdminClient()` method in the NamespacesBase, the TenantsBase, the ClustersBase, and the TransactionsBase. My one concern with the proposed solution is how it handles back pressure. While the current solution has its weaknesses, one of its benefits is that it has back pressure at the request level in the http server. Do we need to think about adding back pressure to ensure that a single call does not dispatch too many internal async calls? I am thinking about calls like `internalDeleteNamespace`, which dispatches futures to delete every topic in the namespace. Note: I do not see any security (authentication/authorization) concerns with this change. The current recursive calls to the same broker are performed by a pulsar client that needs to be configured with a super user token in order to work when authentication and authorization are enabled. Therefore, if the client has sufficient permission to perform the operation, it is assumed to have permission for the calls that result from it. Thanks, Michael On Wed, Jul 6, 2022 at 9:42 PM Anon Hxy <anonhx...@gmail.com> wrote: > > There is a conflict in PIP numbers. The issue has moved to PIP-183. > > Also there is not only one method that should be modified, I will use this > PIP to track all of them. > > > Thanks, > Xiaoyu Hou > > Anon Hxy <anonhx...@gmail.com> 于2022年7月6日周三 22:12写道： > > > Hi Pulsar community: > > > > I open a pip to discuss "Reduce unnecessary REST call in broker" > > > > Proposal Link: https://github.com/apache/pulsar/issues/16422 > > > > --- > > > > ## Motivation > > > > The design of admin API now is such that: when handle a partitioned topic > > request, the broker will query the topic's partition meta, and then use the > > internal admin client to query all the non-partitioned topics (I.e. the > > suffix of the topic name is `-partition-`), > > even if the non-partitioned topic is owned by the broker, which will cause > > unnecessary REST call in the broker. > > > > we can call the methods directlly, who handle the non-partitioned topic, > > to reduce the unnecessary REST call. > > > > ## Goal > > > > * Try to call the methods directlly if the non-partitioned topic is owned > > by the broker > > > > ## Implementation > > > > * We need to check all the place where > > `org.apache.pulsar.broker.PulsarService#getAdminClient` is invoked in > > `org.apache.pulsar.broker.admin.impl.PersistentTopicsBase` > > * take `internalGetPartitionedStats` for example: > > > > * Original: > > ``` > > for (int i = 0; i < partitionMetadata.partitions; i++) { > > try { > > topicStatsFutureList > > > > .add(pulsar().getAdminClient().topics().getStatsAsync( > > > > (topicName.getPartition(i).toString()), getPreciseBacklog, > > subscriptionBacklogSize, > > getEarliestTimeInBacklog)); > > } catch (PulsarServerException e) { > > asyncResponse.resume(new RestException(e)); > > return; > > } > > } > > ``` > > > > * Suggest to do like this: > > ``` > > for (int i = 0; i < partitionMetadata.partitions; i++) { > > TopicName topicNamePartition = topicName.getPartition(i); > > topicStatsFutureList.add( > > > > pulsar().getNamespaceService().isServiceUnitOwnedAsync(topicName) > > .thenCompose(owned -> { > > if (owned) { > > // local call > > return > > getTopicReferenceAsync(topicNamePartition) > > .thenCompose(topic -> > > > > topic.asyncGetStats(getPreciseBacklog, subscriptionBacklogSize, > > getEarliestTimeInBacklog)); > > } else { > > // call from admin client > > try { > > > > pulsar().getAdminClient().topics().getStatsAsync(topicNamePartition.toString()), > > getPreciseBacklog, > > subscriptionBacklogSize, getEarliestTimeInBacklog) > > } catch (PulsarServerException e) { > > throw new RestException(e); > > } > > } > > }) > > ); > > ``` > > > > Thanks, > > Xiaoyu Hou > > > > > >
