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

[improve][consumer] Optimizing Exception Handling and Logging in MultiTopicsConsumerImpl #23568

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ZhaoGuorui666
Copy link
Contributor

Motivation

  1. Generic Exception Handling:

    • The internalReceive method currently catches all exceptions, which can obscure the root cause of specific failures, such as invalid message types.
  2. Insufficient Logging for Validation Failures:

    • When Preconditions.checkArgument fails (throwing an IllegalArgumentException), the existing implementation does not log detailed information about the failure. This limitation makes it challenging to diagnose issues related to message type mismatches.

Modifications

  • Enhanced Exception Handling: Added a specific catch block for IllegalArgumentException to log detailed information before rethrowing as a PulsarClientException.

Verifying this change

Documentation

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

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 6, 2024
@@ -383,6 +383,12 @@ protected Message<T> internalReceive() throws PulsarClientException {
unAckedMessageTracker.add(message.getMessageId(), message.getRedeliveryCount());
resumeReceivingFromPausedConsumersIfNeeded();
return beforeConsume(message);
} catch (IllegalArgumentException e) {
Copy link
Member

Choose a reason for hiding this comment

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

It looks checkState(message instanceof TopicMessageImpl); will throw this exception.

When will this happen? Could you shared your case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants