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

[Issue 664] fix ReconsumeLater panic #753

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

crossoverJie
Copy link
Member

@crossoverJie crossoverJie commented Mar 25, 2022

Fixes #664

Motivation

Removing possible panic.

Modifications

Add nil check and more friendly tips.

Verifying this change

  • Make sure that the change passes the CI checks.

@crossoverJie
Copy link
Member Author

@shoothzj would you mind enabling the workflows and reviewing them?

@shileiyu
Copy link
Contributor

Calling ReconsumeLater without a DLQ policy doesn't make sense in most cases. that sounds some messages could be stuck in an infinite loop forever.

@crossoverJie
Copy link
Member Author

@shileiyu Yes, I agree.

However, there may be careless developers like me who cause the program to run with panic which can even affect the business, this may cut some losses.

@shileiyu
Copy link
Contributor

I’m thinking how could we have a better interface to help developers actively avoiding the pitfall.

@shileiyu
Copy link
Contributor

shileiyu commented Apr 1, 2022

@crossoverJie do you think having a default DLQ policy would be a better solution to this issue?

@crossoverJie
Copy link
Member Author

@shileiyu It's a good idea.
If RetryEnable==true in the Java SDK, a default policy is created.
image

pulsar/consumer_test.go Outdated Show resolved Hide resolved
pulsar/consumer_test.go Outdated Show resolved Hide resolved
@wolfstudy wolfstudy added this to the v0.9.0 milestone Apr 13, 2022
@wolfstudy
Copy link
Member

@shileiyu It's a good idea. If RetryEnable==true in the Java SDK, a default policy is created. image

In Go SDK, we have also made the same settings. When retry is enabled, a default DLQ policy will be created. The essential reason for this panic is that we have not enabled retry, but are trying to use ReconsumeLater

if options.DLQ == nil {
			options.DLQ = &DLQPolicy{
				MaxDeliveries:    MaxReconsumeTimes,
				DeadLetterTopic:  dlqTopic,
				RetryLetterTopic: retryTopic,
			}
		}

Copy link
Member

@wolfstudy wolfstudy left a comment

Choose a reason for hiding this comment

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

LGTM +1

@freeznet freeznet modified the milestones: v0.9.0, v0.10.0 Jul 4, 2022
@RobertIndie RobertIndie modified the milestones: v0.10.0, v0.11.0 Mar 27, 2023
@RobertIndie RobertIndie modified the milestones: v0.11.0, v0.12.0 Jul 4, 2023
@RobertIndie RobertIndie modified the milestones: v0.12.0, v0.13.0 Jan 10, 2024
@RobertIndie RobertIndie modified the milestones: v0.13.0, v0.14.0 Jul 15, 2024
@RobertIndie RobertIndie modified the milestones: v0.14.0, v0.15.0 Oct 8, 2024
# Conflicts:
#	pulsar/consumer_test.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ReconsumeLater cause panic
5 participants