liangyepianzhou commented on PR #24293:
URL: https://github.com/apache/pulsar/pull/24293#issuecomment-2876456286
> I agree with @AuroraTwinkle.
>
> It's not a public API like methods in `pulsar-client-api` though it's a
public method. Adding the `Async` suffix everywhere breaks the beauty of the
code.
>
> I think the root cause is that `NamespaceService` is designed to have both
asynchronous and synchronous methods, so it adopts similar naming rules with
the client APIs.
>
> If I reviewed this PR in time, I would definitely leave a requested change
to prevent merging it. The reason is that once such a PR was merged, there
could be much more meaningless PRs doing the similar thing.
>
> ```java
> % grep "public CompletableFuture"
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java
| grep -v Async
> public CompletableFuture<String> getHeartbeatOrSLAMonitorBrokerId(
> public CompletableFuture<LookupResult> createLookupResult(String
candidateBroker, boolean authoritativeRedirect,
> public CompletableFuture<Void> unloadNamespaceBundle(NamespaceBundle
bundle) {
> public CompletableFuture<Void> unloadNamespaceBundle(NamespaceBundle
bundle, Optional<String> destinationBroker) {
> public CompletableFuture<Void> unloadNamespaceBundle(NamespaceBundle
bundle,
> public CompletableFuture<Void> unloadNamespaceBundle(NamespaceBundle
bundle,
> public CompletableFuture<Void> unloadNamespaceBundle(NamespaceBundle
bundle,
> public CompletableFuture<Void> unloadNamespaceBundle(NamespaceBundle
bundle,
> public CompletableFuture<Boolean>
isNamespaceBundleOwned(NamespaceBundle bundle) {
> public CompletableFuture<Void> splitAndOwnBundle(NamespaceBundle
bundle, boolean unload,
> public CompletableFuture<Pair<NamespaceBundles,
List<NamespaceBundle>>> getSplitBoundary(
> public CompletableFuture<List<Long>> getSplitBoundary(
> public CompletableFuture<Void>
updateNamespaceBundlesForPolicies(NamespaceName nsname,
> public CompletableFuture<Void> updateNamespaceBundles(NamespaceName
nsname, NamespaceBundles nsBundles) {
> public CompletableFuture<Boolean> checkTopicOwnership(TopicName
topicName) {
> public CompletableFuture<List<String>>
getFullListOfTopics(NamespaceName namespaceName) {
> public CompletableFuture<List<String>>
getFullListOfPartitionedTopic(NamespaceName namespaceName) {
> public CompletableFuture<List<String>>
getOwnedTopicListForNamespaceBundle(NamespaceBundle bundle) {
> public CompletableFuture<TopicExistsInfo> checkTopicExists(TopicName
topic) {
> public CompletableFuture<Boolean>
checkNonPartitionedTopicExists(TopicName topic) {
> public CompletableFuture<Boolean>
checkNonPersistentNonPartitionedTopicExists(String topic) {
> public CompletableFuture<List<String>> getListOfTopics(NamespaceName
namespaceName, Mode mode) {
> public CompletableFuture<List<String>>
getListOfUserTopics(NamespaceName namespaceName, Mode mode) {
> public CompletableFuture<List<String>> getAllPartitions(NamespaceName
namespaceName) {
> public CompletableFuture<List<String>> getPartitions(NamespaceName
namespaceName, TopicDomain topicDomain) {
> public CompletableFuture<List<String>>
getListOfPersistentTopics(NamespaceName namespaceName) {
> public CompletableFuture<List<String>>
getListOfNonPersistentTopics(NamespaceName namespaceName) {
> ```
>
> We should avoid encouraging new guys pushing similar PRs like:
>
> * Add `Async` to `getListOfTopics`
> * Add `Async` to `getListOfUserTopics`
> * ...
@AuroraTwinkle @BewareMyPower Thank you for the valuable feedback! 🙏 The
original intention of this PR was indeed to address a specific issue I
identified during code review (https://github.com/apache/pulsar/pull/24225
regarding asynchronous methods directly calling synchronously named methods),
but your perspectives have helped me realize the need to consider
standardization from a higher-level approach. There's one point I completely
agree with: systematic standardization should take precedence over localized
fixes. For similar issues in the future, we should file GitHub issues to track
them and address them collectively after establishing clear guidelines.
--
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]
