You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "liangyepianzhou (via GitHub)" <gi...@apache.org> on 2025/05/13 02:15:25 UTC
[PR] [fix][broker] fix wrong method name checkTopicExists. [pulsar]
liangyepianzhou opened a new pull request, #24293:
URL: https://github.com/apache/pulsar/pull/24293
### 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.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
*(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:)*
- *Added integration tests for end-to-end deployment with large payloads (10MB)*
- *Extended integration test for recovery after broker failure*
### Does this pull request potentially affect one of the following parts:
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
*If the box was checked, please highlight the changes*
- [ ] Dependencies (add or upgrade a dependency)
- [ ] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The threading model
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] The metrics
- [ ] Anything that affects deployment
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository: <!-- ENTER URL HERE -->
<!--
After opening this PR, the build in apache/pulsar will fail and instructions will
be provided for opening a PR in the PR author's forked repository.
apache/pulsar pull requests should be first tested in your own fork since the
apache/pulsar CI based on GitHub Actions has constrained resources and quota.
GitHub Actions provides separate quota for pull requests that are executed in
a forked repository.
The tests will be run in the forked repository until all PR review comments have
been handled, the tests pass and the PR is approved by a reviewer.
-->
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [refactor][broker] fix wrong method name checkTopicExists. [pulsar]
Posted by "liangyepianzhou (via GitHub)" <gi...@apache.org>.
liangyepianzhou commented on PR #24293:
URL: https://github.com/apache/pulsar/pull/24293#issuecomment-2876455036
> 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 `@Deprecated`.
>
> 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)
@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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] fix wrong method name checkTopicExists. [pulsar]
Posted by "AuroraTwinkle (via GitHub)" <gi...@apache.org>.
AuroraTwinkle commented on PR #24293:
URL: https://github.com/apache/pulsar/pull/24293#issuecomment-2876180899
> > Why don't other methods add the Async suffix but just `checkTopicExists`? For example: `checkNonPartitionedTopicExists`
>
> @AuroraTwinkle While reviewing #24225, I noticed an issue where the newly added method `checkTopicExistsAsync` simply calls the synchronous `checkTopicExists`. This pattern is confusing, and since the PR author did not intend to address it, I resolved the problem myself.
>
> 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
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:
1. Is it necessary to stipulate that the name of the asynchronous method should be suffixed with Async (in my opinion, there is no need to force the Async suffix)
2. If necessary, then a related check style should be established to prevent non-standard code submission
3. Existing methods should remain unchanged: If you just add a new method signature with the Async suffix, I think it doesnโt make much sense and will greatly destroy the beauty of the code (Because there are many existing asynchronous methods without the Async suffix).
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] fix wrong method name checkTopicExists. [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #24293:
URL: https://github.com/apache/pulsar/pull/24293#issuecomment-2876333380
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`
- ...
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [refactor][broker] fix wrong method name checkTopicExists. [pulsar]
Posted by "liangyepianzhou (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] fix wrong method name checkTopicExists. [pulsar]
Posted by "AuroraTwinkle (via GitHub)" <gi...@apache.org>.
AuroraTwinkle commented on PR #24293:
URL: https://github.com/apache/pulsar/pull/24293#issuecomment-2876018630
Why don't other methods add the Async suffix? For example: `checkNonPartitionedTopicExists`
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] fix wrong method name checkTopicExists. [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #24293:
URL: https://github.com/apache/pulsar/pull/24293#issuecomment-2876352658
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 `@Deprecated`.
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: https://github.com/apache/pulsar/pull/24290)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] fix wrong method name checkTopicExists. [pulsar]
Posted by "liangyepianzhou (via GitHub)" <gi...@apache.org>.
liangyepianzhou merged PR #24293:
URL: https://github.com/apache/pulsar/pull/24293
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix][broker] fix wrong method name checkTopicExists. [pulsar]
Posted by "liangyepianzhou (via GitHub)" <gi...@apache.org>.
liangyepianzhou commented on PR #24293:
URL: https://github.com/apache/pulsar/pull/24293#issuecomment-2876060898
> Why don't other methods add the Async suffix but just `checkTopicExists`? For example: `checkNonPartitionedTopicExists`
@AuroraTwinkle While reviewing https://github.com/apache/pulsar/pull/24225, I noticed an issue where a new method `checkTopicExistsAsync` was added but merely wraps `checkTopicExists`. This implementation approach confused me, and since the PR author had no intention of fixing this together, I went ahead and resolved it.
If you'd like, you could check if there are other similar asynchronous-to-synchronous wraps in Pulsar 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.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org