Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix] [broker] print non log when delete partitioned topic failed #22153

Conversation

poorbarcode
Copy link
Contributor

@poorbarcode poorbarcode commented Feb 28, 2024

Motivation

https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java#L2968-L2983

replCloseFuture.thenCompose(v -> delete())
  .thenApply((res) -> tryToDeletePartitionedMetadata()) // This line lost the error throws by "tryToDeletePartitionedMetadata"
  .exceptionally(e -> {
    log.warn("[{}] Inactive topic deletion failed", topic, e);
   });
}

The line thenApply((res) -> tryToDeletePartitionedMetadata()) lost the error throws by "tryToDeletePartitionedMetadata", makes these two issues:

  • No log was printed when deleting partitioned metadata failed.
  • The future of delete the topic and delete partitioned topic metadata if needed only indicates delete topic partition, not delete topic and metadata. BTW, this future was not used anywhere.

Modifications

  • Fix the issue
  • Print an info level log if the partitioned metadata should not be deleted.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: x

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this impacts more than printing the log message. Switching from thenApply to thenComplete will also change when the resulting CompetableFuture completes?
If this is the case, please reflect this in the PR title and the description.

UPDATE: it seems that before, the log "Topic deleted successfully due to inactivity" could get printed before the operation had completed since thenApply was used.

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work @Technoboy-

@poorbarcode
Copy link
Contributor Author

poorbarcode commented Feb 29, 2024

@lhotari

UPDATE: it seems that before, the log "Topic deleted successfully due to inactivity" could get printed before the operation had completed since thenApply was used.

Added this info into the Motivation, thanks

Screenshot 2024-02-29 at 12 45 43

@poorbarcode
Copy link
Contributor Author

Merging

@poorbarcode poorbarcode merged commit 72cedb7 into apache:master Feb 29, 2024
66 of 69 checks passed
gaoran10 pushed a commit that referenced this pull request Mar 4, 2024
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 4, 2024
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants