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: use ctx and timer instead sleep #1256

Merged
merged 2 commits into from
Nov 15, 2024

Conversation

nodece
Copy link
Member

@nodece nodece commented Jul 25, 2024

Motivation

We are using time.Sleep and for to execute the retry, which cannot be interrupted, this PR will use context and timer to improve this behavior.

The next improvement idea supports passing the context to interrupt the retry, when closing the producer or consumer, we need to do that.

Modifications

  • Add the Retry method to execute the try, and use Retry instead of time.Sleep and for.

Copy link
Member

@RobertIndie RobertIndie left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. Left one comment.

pulsar/consumer_partition.go Show resolved Hide resolved
@nodece nodece requested a review from RobertIndie July 31, 2024 04:08
@nodece nodece force-pushed the user-ctx-timer-instead-sleep branch 2 times, most recently from 8d8b6f2 to d32ac75 Compare July 31, 2024 06:44
@nodece
Copy link
Member Author

nodece commented Jul 31, 2024

Let me fix the consumer seek test first.

@nodece
Copy link
Member Author

nodece commented Aug 14, 2024

Seek test will be fixed by #1265.

@RobertIndie RobertIndie modified the milestones: v0.14.0, v0.15.0 Oct 8, 2024
@nodece nodece force-pushed the user-ctx-timer-instead-sleep branch from e307794 to f7caa08 Compare November 7, 2024 09:26
@nodece nodece force-pushed the user-ctx-timer-instead-sleep branch from f7caa08 to 9904d77 Compare November 7, 2024 09:33
@nodece
Copy link
Member Author

nodece commented Nov 8, 2024

@RobertIndie @crossoverJie Could you have a chance to review?

Copy link
Member

@crossoverJie crossoverJie left a comment

Choose a reason for hiding this comment

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

LGTM

@nodece nodece merged commit 92c6e28 into apache:master Nov 15, 2024
7 checks passed
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.

3 participants