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 > > > > > > > > >
