Hi, that looks good. There are also some REST calls that can be reduced.
As: When unloadNamespace, if a bund on this broker, can be directly unloaded. Thanks, Baodi Shi > On Jul 7, 2022, at 15:3330, Enrico Olivelli <eolive...@gmail.com> wrote: > > Xiaoyu Hou, > I support this PIP > > Enrico > > Il giorno gio 7 lug 2022 alle ore 05:37 Michael Marshall > <mmarsh...@apache.org> ha scritto: >> >> 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 >>>> >>>> >>>>
