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] Do not record as a topic load failure for the requested broker is not the owner #23458

Conversation

codelipenghui
Copy link
Contributor

@codelipenghui codelipenghui commented Oct 14, 2024

Motivation

The topic_load_failed metric is introduced by #19236 which is used to monitor the topic load failures such as timeout, metadata service issue, etc.

But the topic owner check exception is an expected behavior if the requested broker is not the owner of the topic. We should avoid to record as a topic load failure from topic owner check.

For the non-persistent topic, we don't need this change.

}).exceptionally(e -> {
log.warn("CheckTopicNsOwnership fail when createNonPersistentTopic! {}", topic, e.getCause());
// CheckTopicNsOwnership fail dont create nonPersistentTopic, when topic do lookup will find the correct
// broker. When client get non-persistent-partitioned topic
// metadata will the non-persistent-topic will be created.
// so we should add checkTopicNsOwnership logic otherwise the topic will be created
// if it dont own by this broker,we should return success
// otherwise it will keep retrying getPartitionedTopicMetadata
topicFuture.complete(Optional.of(nonPersistentTopic));
// after get metadata return success, we should delete this topic from this broker, because this topic not
// owner by this broker and it don't initialize and checkReplication
pulsar.getExecutor().execute(() -> topics.remove(topic, topicFuture));
return null;
});

Verifying this change

  • New unit tests were added

Does this pull request potentially affect one of the following parts:

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

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

Matching PR in forked repository

PR in forked repository:

@codelipenghui codelipenghui self-assigned this Oct 14, 2024
@codelipenghui codelipenghui added this to the 4.1.0 milestone Oct 14, 2024
@codelipenghui codelipenghui added type/bug The PR fixed a bug or issue reported a bug area/broker area/metrics labels Oct 14, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 14, 2024
@codelipenghui
Copy link
Contributor Author

There is a PIP to improve this case #23351, which is better than just handle the ServiceNotReady exception. I will close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker area/metrics doc-not-needed Your PR changes do not impact docs release/3.0.8 release/3.3.3 release/4.0.1 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants