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.

Sign up for GitHub

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

Jump to bottom

GH-2588: ARBP support batch recoverable #2888

Merged
merged 4 commits into from
Feb 5, 2024
Merged

GH-2588: ARBP support batch recoverable #2888

merged 4 commits into from
Feb 5, 2024

Conversation

Wzy19930507
Copy link
Contributor

@Wzy19930507 Wzy19930507 commented Nov 9, 2023 •
edited
Loading

Resolves #2588

This is a draft of support DLT batch recover for DefaultAfterRollbackProcessor. so we can evaluate if it's going in the right direction before doing a final review, adding docs, etc.

Function

  • add opt-in property batchRecoverAfterRollback at ContainerProperties
  • add method processBatch at AfterRollbackProcessor

Batch recoverable

  • batchRecoverAfterRollback is true
  • set transcation DLT to enable isCommitRecovered
  • enable kafka transcation

exception handler logic

  • in retry, seek to begin
  • out of retry exception recover entire recods
  • recover fail, seek to begin

@garyrussell
Copy link
Contributor

Can you provide a valid use case where this would be useful?

Any transactional sends for the records before the failed one would have been rolled back, so it doesn’t make sense to to skip those records on the retry. If you are not doing transactional sends, then why use transactions?

I tried to convey this in my comments on the issue.

@Wzy19930507
Copy link
Contributor Author

Wzy19930507 commented Nov 10, 2023 •
edited
Loading

Any transactional sends for the records before the failed one would have been rolled back, so it doesn’t make sense to to skip those records on the retry. If you are not doing transactional sends, then why use transactions?

Exactly right, this is the most important point in this issue.

@garyrussell
Copy link
Contributor

One more time - we cannot support BatchListenerFailedException here - the transaction will have rolled back by now so any transactional sends for the records before the failed record will have been ignored; it is not correct to discard those records.

Yes, we could support recovering (sending to DLT) the entire batch, but we can't discard a partial batch here. Even that would have to be an opt-in feature.

@Wzy19930507 Wzy19930507 changed the title GH-2588: ARBP support BatchListenerFailedException GH-2588: ARBP support batch recoverable Nov 17, 2023
@Wzy19930507

This comment was marked as resolved.

Sign in to view
@garyrussell
Copy link
Contributor

Re-running now; that test is sometimes flaky.

@Wzy19930507
Copy link
Contributor Author

Thanks

@artembilan
Copy link
Member

@Wzy19930507 ,

please, rebase your branch to the latest main.

Then we will start looking into your change to incorporate it into a new version.

Thanks

artembilan
artembilan requested changes Jan 18, 2024
View reviewed changes
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.

Don't we also need to document the feature somehow?

Thanks

spring-kafka/src/main/java/org/springframework/kafka/listener/ContainerProperties.java Outdated Show resolved Hide resolved
...ng-kafka/src/main/java/org/springframework/kafka/listener/DefaultAfterRollbackProcessor.java Outdated Show resolved Hide resolved
spring-kafka/src/main/java/org/springframework/kafka/listener/ListenerUtils.java Outdated Show resolved Hide resolved
spring-kafka/src/main/java/org/springframework/kafka/listener/SeekUtils.java Show resolved Hide resolved
spring-kafka/src/test/java/org/springframework/kafka/listener/CommitOnAssignmentTests.java Outdated Show resolved Hide resolved
artembilan
artembilan requested changes Jan 19, 2024
View reviewed changes
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.

Please, also add your name to the @author list of all the affected classes.

Thanks

spring-kafka-docs/src/main/antora/modules/ROOT/pages/kafka/annotation-error-handling.adoc Outdated Show resolved Hide resolved
spring-kafka/src/main/java/org/springframework/kafka/listener/AfterRollbackProcessor.java Show resolved Hide resolved
spring-kafka/src/main/java/org/springframework/kafka/listener/ContainerProperties.java Show resolved Hide resolved
...ng-kafka/src/main/java/org/springframework/kafka/listener/DefaultAfterRollbackProcessor.java Show resolved Hide resolved
spring-kafka/src/main/java/org/springframework/kafka/listener/ListenerUtils.java Show resolved Hide resolved
spring-kafka/src/main/java/org/springframework/kafka/listener/SeekUtils.java Show resolved Hide resolved
Zhiyang.Wang1 and others added 4 commits February 2, 2024 20:34
@Wzy19930507
spring-projectsGH-2588: support batch recoverable `DefaultAfterRollba…
97139d5 
…ckProcessor`

* add method processBatch at `AfterRollbackProcessor`

* add opt-in property `batchRecoverAfterRollback` at `ContainerProperties`

* change format to `BatchListenerFailedException.getMessage`

* add batch recoverable after rollback unit test
@Wzy19930507
review fix
8b8ed18 
* `what-new.adoc` and `annotation-error-handling.adoc`
* add javadoc in `SeekUtils` and `AfterRollbackProcessor`
* change `ListenerUtils.nextBackOff` public to default
* change logger args to static string
@Wzy19930507
review fix
58528e0 
* @author classes.
* fix adoc
* poblish `AfterRollbackProcessor`
* javadoc at `ContainerProperties`
@Wzy19930507
fix review and fix test bug at DefaultAfterRollbackProcessorTests
8886769 
* add @test to DefaultAfterRollbackProcessorTests.testNoEarlyExitBackOff
* polish TransactionalContainerTests
* fix bug Tests at DefaultAfterRollbackProcessorTests method `testNoEarlyExitBackOff` and testEarlyExitBackOff
@sobychacko sobychacko merged commit 630dbb2 into spring-projects:main Feb 5, 2024
3 checks passed
@Wzy19930507 Wzy19930507 deleted the GH-2588 branch February 6, 2024 02:49
@Wzy19930507
Copy link
Contributor Author

Wzy19930507 commented Feb 6, 2024 •
edited
Loading

@artembilan @garyrussell @sobychacko Thanks for review, this issue could not have been solved without your guidance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@artembilan artembilan artembilan requested changes

@sobychacko sobychacko sobychacko approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Provide ability to disable/limit retries in batch listener when using DefaultAfterRollbackProcessor
4 participants
@Wzy19930507 @garyrussell @artembilan @sobychacko