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

GH-3166: BatchInterceptor issues with retries #3167

Merged
merged 2 commits into from
Mar 28, 2024

Conversation

sobychacko
Copy link
Contributor

Fixes: #3166

  • When retries are enabled, batch interceptor is not invoking the intercept methods for failures on retries and the possible eventual success method call. Addressing this issue.

Fixes: spring-projects#3166

* When retries are enabled, batch interceptor is not
  invoking the intercept methods for failures on retries
  and the possible eventual success method call.
  Addressing this issue.
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

Just one minor nit-pick in the new unit test.

I guess we are still going to back-port the fix into all our supported version?

given(consumer.poll(any(Duration.class))).willAnswer(i -> {
Thread.sleep(50);
if (invocation.get() == 0) {
invocation.getAndIncrement();
Copy link
Member

Choose a reason for hiding this comment

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

This definitely can be a part of that if: no need to call get and then getAndIncrement. The last one would contribute exactly same value for that if.
Also I'm not sure that Thread.sleep(50). Even if it is small enough, we still need to be sure that we need such a delay.
Why to make test slower for nothing?

@sobychacko
Copy link
Contributor Author

@artembilan I added the backport labels to the original issue. I will tackle the test comments soon.

Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

LGTM.

Will merge when build is green.

@artembilan artembilan merged commit c26f26e into spring-projects:main Mar 28, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BatchInterceptor is not working properly with retries
2 participants