Good job! +1 Baodi Shi <baodi....@icloud.com.invalid> 于2022年7月7日周四 22:16写道：
> 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 > >>>> > >>>> > >>>> > > -- BR, Qiang Huang
