BewareMyPower commented on code in PR #23248: URL: https://github.com/apache/pulsar/pull/23248#discussion_r1742889487
########## pip/pip-376.md: ########## @@ -0,0 +1,138 @@ +# PIP-376: Make topic policies service pluggable + +# Background knowledge + +## Topic policies service and system topics + +[PIP-39](https://github.com/apache/pulsar/wiki/PIP-39%3A-Namespace-Change-Events) introduces system topics and the topic level policies. However, the topic policies service (`TopicPoliciesService`) only has one implementation (`SystemTopicBasedTopicPoliciesService`) that depends on the system topics. So the following configs are both required (though they're all enabled by default now): + +```properties +systemTopicEnabled=true +topicLevelPoliciesEnabled=true +``` + +However, if the Pulsar storage is switched to a S3-based solution (by modifying the `managedLedgerStorageClassName` config), using system topics to manage topic policies could have low performance (due to the S3 write and read latency) and higher cost (due to redundant S3 API calls). + +## Badly designed TopicPoliciesService interface + +The `TopicPoliciesService` interface is a terrible abstraction because it's never designed for 3rd party implementations. + +1. Methods that should not be exposed + +`addOwnedNamespaceBundleAsync` and `removeOwnedNamespaceBundleAsync` are only used internally in `SystemTopicBasedTopicPoliciesService`. + +`getTopicPoliciesBypassCacheAsync` is only used in tests. This method just creates a reader to replay the `__change_events` topic and construct the topic policies map. + +2. Confusing and inconsistent `getTopicPolicies` family + +There are two overrides of `getTopicPolicies`: + +```java +TopicPolicies getTopicPolicies(TopicName topicName, boolean isGlobal) throws TopicPoliciesCacheNotInitException; +TopicPolicies getTopicPolicies(TopicName topicName) throws TopicPoliciesCacheNotInitException; +``` + +The 2nd method is equivalent to `getTopicPolicies(topicName, false)`. + +The semantics of these two methods are very intuitive. First, they are not synchronous methods that are blocked by waiting a future. They just start an asynchronous policies initialization (creating a reader to replay the `__change_events` topic), and then try to get the policies from the cache. If the asynchronous policies initialization didn't start, just throw `TopicPoliciesCacheNotInitException`. + +As you can see, these two methods are hard to use. And they are also only used in tests except for the `getTopicPoliciesAsyncWithRetry` method, which uses a user-provided executor and backoff policy to call `getTopicPolicies` until `TopicPoliciesCacheNotInitException` is not thrown: + +```java + default CompletableFuture<Optional<TopicPolicies>> getTopicPoliciesAsyncWithRetry(TopicName topicName, + final Backoff backoff, ScheduledExecutorService scheduledExecutorService, boolean isGlobal) { +``` Review Comment: Yeah, this section just talks about the existing interface. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
