[refactor][broker] fix wrong method name checkTopicExists. #24293
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Why don't other methods add the Async suffix but just |
Contributor
Author
@AuroraTwinkle While reviewing #24225, I noticed an issue where the newly added method If you'd like, could you check whether there are other instances in Pulsar where asynchronous methods improperly reuse synchronous method names (or vice versa)** and fix them? If you don't have time, I can take some time next week to do a deeper inspection of the Pulsar codebase and fix similar patterns |
Contributor
In fact, what I want to say is that I don't think it's necessary to add the Async suffix to existing methods, especially those that are not client-side methods. This will result in many existing functions having two signatures (one with the Async suffix and the other without), which is more likely to cause misunderstandings and destroy the beauty of the code. Perhaps what should be discussed is:
|
Contributor
|
I agree with @AuroraTwinkle. It's not a public API like methods in I think the root cause is that 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. % 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:
|
Contributor
|
In addition, this PR should never be a "fix". Regarding the unused public methods, currently I think we can just remove them directly without marking the old methods as I started a discussion long days ago: https://lists.apache.org/thread/v4bswc3byqtvpl3h4cjpvzm8sbsy0wq9 We should not waste much time keeping compatibility for any public method because a public method is added too casually (that might be also why there is such a PIP: #24290) |
Contributor
Author
@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 (#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. |
lhotari
reviewed
May 13, 2025
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java
Show resolved
Hide resolved
Member
|
Cherry-picking to at least branch-4.0 since this change is fully compatible. It would make cherry-picking more painful going forward if we don't make this change in all maintenance branches. |
lhotari
pushed a commit
that referenced
this pull request
May 16, 2025
The current method `checkTopicExists` is an asynchronous method but follows a synchronous naming convention (lacking the `Async` suffix). This naming inconsistency can mislead developers into assuming it's a blocking operation, potentially causing misuse in client code. Since this method is `public`, we cannot remove it directly without breaking backward compatibility. 1. **Introduce a new asynchronous method**: - Added `checkTopicExistsAsync()` with the correct asynchronous naming convention. - Internally delegates to the original `checkTopicExists()` method to retain existing logic. 2. **Deprecate the original method**: - Marked `checkTopicExists()` as `@Deprecated` with a note directing users to the new `checkTopicExistsAsync()`. 3. **Refactor internal usages**: - Updated all internal calls to use `checkTopicExistsAsync()` instead of the deprecated method. 4. **Documentation updates**: - Added Javadoc to `checkTopicExists()` clarifying its deprecated status and replacement. This approach maintains backward compatibility while aligning method names with their asynchronous behavior. (cherry picked from commit af24849)
lhotari
pushed a commit
that referenced
this pull request
May 16, 2025
The current method `checkTopicExists` is an asynchronous method but follows a synchronous naming convention (lacking the `Async` suffix). This naming inconsistency can mislead developers into assuming it's a blocking operation, potentially causing misuse in client code. Since this method is `public`, we cannot remove it directly without breaking backward compatibility. 1. **Introduce a new asynchronous method**: - Added `checkTopicExistsAsync()` with the correct asynchronous naming convention. - Internally delegates to the original `checkTopicExists()` method to retain existing logic. 2. **Deprecate the original method**: - Marked `checkTopicExists()` as `@Deprecated` with a note directing users to the new `checkTopicExistsAsync()`. 3. **Refactor internal usages**: - Updated all internal calls to use `checkTopicExistsAsync()` instead of the deprecated method. 4. **Documentation updates**: - Added Javadoc to `checkTopicExists()` clarifying its deprecated status and replacement. This approach maintains backward compatibility while aligning method names with their asynchronous behavior. (cherry picked from commit af24849)
lhotari
pushed a commit
that referenced
this pull request
May 16, 2025
The current method `checkTopicExists` is an asynchronous method but follows a synchronous naming convention (lacking the `Async` suffix). This naming inconsistency can mislead developers into assuming it's a blocking operation, potentially causing misuse in client code. Since this method is `public`, we cannot remove it directly without breaking backward compatibility. 1. **Introduce a new asynchronous method**: - Added `checkTopicExistsAsync()` with the correct asynchronous naming convention. - Internally delegates to the original `checkTopicExists()` method to retain existing logic. 2. **Deprecate the original method**: - Marked `checkTopicExists()` as `@Deprecated` with a note directing users to the new `checkTopicExistsAsync()`. 3. **Refactor internal usages**: - Updated all internal calls to use `checkTopicExistsAsync()` instead of the deprecated method. 4. **Documentation updates**: - Added Javadoc to `checkTopicExists()` clarifying its deprecated status and replacement. This approach maintains backward compatibility while aligning method names with their asynchronous behavior. (cherry picked from commit af24849)
ganesh-ctds
pushed a commit
to datastax/pulsar
that referenced
this pull request
May 20, 2025
The current method `checkTopicExists` is an asynchronous method but follows a synchronous naming convention (lacking the `Async` suffix). This naming inconsistency can mislead developers into assuming it's a blocking operation, potentially causing misuse in client code. Since this method is `public`, we cannot remove it directly without breaking backward compatibility. 1. **Introduce a new asynchronous method**: - Added `checkTopicExistsAsync()` with the correct asynchronous naming convention. - Internally delegates to the original `checkTopicExists()` method to retain existing logic. 2. **Deprecate the original method**: - Marked `checkTopicExists()` as `@Deprecated` with a note directing users to the new `checkTopicExistsAsync()`. 3. **Refactor internal usages**: - Updated all internal calls to use `checkTopicExistsAsync()` instead of the deprecated method. 4. **Documentation updates**: - Added Javadoc to `checkTopicExists()` clarifying its deprecated status and replacement. This approach maintains backward compatibility while aligning method names with their asynchronous behavior. (cherry picked from commit af24849) (cherry picked from commit 8596e6a)
srinath-ctds
pushed a commit
to datastax/pulsar
that referenced
this pull request
May 21, 2025
The current method `checkTopicExists` is an asynchronous method but follows a synchronous naming convention (lacking the `Async` suffix). This naming inconsistency can mislead developers into assuming it's a blocking operation, potentially causing misuse in client code. Since this method is `public`, we cannot remove it directly without breaking backward compatibility. 1. **Introduce a new asynchronous method**: - Added `checkTopicExistsAsync()` with the correct asynchronous naming convention. - Internally delegates to the original `checkTopicExists()` method to retain existing logic. 2. **Deprecate the original method**: - Marked `checkTopicExists()` as `@Deprecated` with a note directing users to the new `checkTopicExistsAsync()`. 3. **Refactor internal usages**: - Updated all internal calls to use `checkTopicExistsAsync()` instead of the deprecated method. 4. **Documentation updates**: - Added Javadoc to `checkTopicExists()` clarifying its deprecated status and replacement. This approach maintains backward compatibility while aligning method names with their asynchronous behavior. (cherry picked from commit af24849) (cherry picked from commit 8596e6a)
manas-ctds
pushed a commit
to datastax/pulsar
that referenced
this pull request
May 28, 2025
The current method `checkTopicExists` is an asynchronous method but follows a synchronous naming convention (lacking the `Async` suffix). This naming inconsistency can mislead developers into assuming it's a blocking operation, potentially causing misuse in client code. Since this method is `public`, we cannot remove it directly without breaking backward compatibility. 1. **Introduce a new asynchronous method**: - Added `checkTopicExistsAsync()` with the correct asynchronous naming convention. - Internally delegates to the original `checkTopicExists()` method to retain existing logic. 2. **Deprecate the original method**: - Marked `checkTopicExists()` as `@Deprecated` with a note directing users to the new `checkTopicExistsAsync()`. 3. **Refactor internal usages**: - Updated all internal calls to use `checkTopicExistsAsync()` instead of the deprecated method. 4. **Documentation updates**: - Added Javadoc to `checkTopicExists()` clarifying its deprecated status and replacement. This approach maintains backward compatibility while aligning method names with their asynchronous behavior. (cherry picked from commit af24849) (cherry picked from commit 86ef0a0)
nodece
pushed a commit
to ascentstream/pulsar
that referenced
this pull request
May 28, 2025
The current method `checkTopicExists` is an asynchronous method but follows a synchronous naming convention (lacking the `Async` suffix). This naming inconsistency can mislead developers into assuming it's a blocking operation, potentially causing misuse in client code. Since this method is `public`, we cannot remove it directly without breaking backward compatibility. 1. **Introduce a new asynchronous method**: - Added `checkTopicExistsAsync()` with the correct asynchronous naming convention. - Internally delegates to the original `checkTopicExists()` method to retain existing logic. 2. **Deprecate the original method**: - Marked `checkTopicExists()` as `@Deprecated` with a note directing users to the new `checkTopicExistsAsync()`. 3. **Refactor internal usages**: - Updated all internal calls to use `checkTopicExistsAsync()` instead of the deprecated method. 4. **Documentation updates**: - Added Javadoc to `checkTopicExists()` clarifying its deprecated status and replacement. This approach maintains backward compatibility while aligning method names with their asynchronous behavior. (cherry picked from commit af24849)
srinath-ctds
pushed a commit
to datastax/pulsar
that referenced
this pull request
Jun 2, 2025
The current method `checkTopicExists` is an asynchronous method but follows a synchronous naming convention (lacking the `Async` suffix). This naming inconsistency can mislead developers into assuming it's a blocking operation, potentially causing misuse in client code. Since this method is `public`, we cannot remove it directly without breaking backward compatibility. 1. **Introduce a new asynchronous method**: - Added `checkTopicExistsAsync()` with the correct asynchronous naming convention. - Internally delegates to the original `checkTopicExists()` method to retain existing logic. 2. **Deprecate the original method**: - Marked `checkTopicExists()` as `@Deprecated` with a note directing users to the new `checkTopicExistsAsync()`. 3. **Refactor internal usages**: - Updated all internal calls to use `checkTopicExistsAsync()` instead of the deprecated method. 4. **Documentation updates**: - Added Javadoc to `checkTopicExists()` clarifying its deprecated status and replacement. This approach maintains backward compatibility while aligning method names with their asynchronous behavior. (cherry picked from commit af24849) (cherry picked from commit 86ef0a0)
KannarFr
pushed a commit
to CleverCloud/pulsar
that referenced
this pull request
Sep 22, 2025
### Motivation The current method `checkTopicExists` is an asynchronous method but follows a synchronous naming convention (lacking the `Async` suffix). This naming inconsistency can mislead developers into assuming it's a blocking operation, potentially causing misuse in client code. Since this method is `public`, we cannot remove it directly without breaking backward compatibility. ### Modifications 1. **Introduce a new asynchronous method**: - Added `checkTopicExistsAsync()` with the correct asynchronous naming convention. - Internally delegates to the original `checkTopicExists()` method to retain existing logic. 2. **Deprecate the original method**: - Marked `checkTopicExists()` as `@Deprecated` with a note directing users to the new `checkTopicExistsAsync()`. 3. **Refactor internal usages**: - Updated all internal calls to use `checkTopicExistsAsync()` instead of the deprecated method. 4. **Documentation updates**: - Added Javadoc to `checkTopicExists()` clarifying its deprecated status and replacement. This approach maintains backward compatibility while aligning method names with their asynchronous behavior.
walkinggo
pushed a commit
to walkinggo/pulsar
that referenced
this pull request
Oct 8, 2025
### Motivation The current method `checkTopicExists` is an asynchronous method but follows a synchronous naming convention (lacking the `Async` suffix). This naming inconsistency can mislead developers into assuming it's a blocking operation, potentially causing misuse in client code. Since this method is `public`, we cannot remove it directly without breaking backward compatibility. ### Modifications 1. **Introduce a new asynchronous method**: - Added `checkTopicExistsAsync()` with the correct asynchronous naming convention. - Internally delegates to the original `checkTopicExists()` method to retain existing logic. 2. **Deprecate the original method**: - Marked `checkTopicExists()` as `@Deprecated` with a note directing users to the new `checkTopicExistsAsync()`. 3. **Refactor internal usages**: - Updated all internal calls to use `checkTopicExistsAsync()` instead of the deprecated method. 4. **Documentation updates**: - Added Javadoc to `checkTopicExists()` clarifying its deprecated status and replacement. This approach maintains backward compatibility while aligning method names with their asynchronous behavior.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
The current method
checkTopicExistsis an asynchronous method but follows a synchronous naming convention (lacking theAsyncsuffix). This naming inconsistency can mislead developers into assuming it's a blocking operation, potentially causing misuse in client code. Since this method ispublic, we cannot remove it directly without breaking backward compatibility.Modifications
Introduce a new asynchronous method:
checkTopicExistsAsync()with the correct asynchronous naming convention.checkTopicExists()method to retain existing logic.Deprecate the original method:
checkTopicExists()as@Deprecatedwith a note directing users to the newcheckTopicExistsAsync().Refactor internal usages:
checkTopicExistsAsync()instead of the deprecated method.Documentation updates:
checkTopicExists()clarifying its deprecated status and replacement.This approach maintains backward compatibility while aligning method names with their asynchronous behavior.
Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: