Fix for issue #2590 #2685

thperchi · 2023-05-17T16:00:10Z

Hello,

I hope this message finds you well. I am writing to submit a pull request for the spring-kafka project to address a fix for the issue #2590.

Here are the details of the pull request:

Repository: https://github.com/INTM-Group/spring-kafka
Issue: Consumer committed offsets not tracking LSO with FixTxOffsets and rollbacked transactions #2590

It has successfully resolved the issue. The changes I made adhere to the existing codebase's style and best practices.

I kindly request you to review and consider merging this pull request into the main branch. I am open to any feedback or suggestions you may have, and I am committed to addressing them promptly.

Thank you for your time and consideration. I appreciate the opportunity to contribute to the spring-kafka project.

Thibault Perchicot

pivotal-cla · 2023-05-17T16:00:14Z

@thperchi Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

garyrussell · 2023-05-17T16:14:25Z

Thanks; but we can't look at your PR until you "sign" the CLA.

Do you think it would be possible to create a test that exhibits the behavior without the patch and passes with it?

pivotal-cla · 2023-05-19T08:16:45Z

@thperchi Thank you for signing the Contributor License Agreement!

thperchi · 2023-05-19T08:20:48Z

Hi !
It should be good for the CLA.
Here is a test that fail without the pach and passes with it : spring-kafka-2590-repro-v2.zip
Thanks for your time.

artembilan

Thank you for your effort, but we don't accept changes without tests confirmation in the project code.
Doesn't look like your change is that minor to make it obvious. Plus I see there is a failing test in GH action for this PR:

TransactionalContainerTests > testRollbackRecord() FAILED
    org.opentest4j.AssertionFailedError: 
    expected: null
     but was: OffsetAndMetadata{offset=0, leaderEpoch=null, metadata=''}
        at [email protected]/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at [email protected]/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
        at [email protected]/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at [email protected]/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499)
        at app//org.springframework.kafka.listener.TransactionalContainerTests.testRollbackRecord(TransactionalContainerTests.java:568)

Some way it feels like this failure is related to your change since both are talking about transactions.

Thanks for understanding!

garyrussell · 2023-05-22T14:30:19Z

To run the tests locally, use ./gradlew clean check.

thperchi · 2023-05-23T12:29:04Z

Thank you for the review, I'm gonna check out how to solve this !

bgK · 2023-05-26T13:18:58Z

...ng-kafka/src/main/java/org/springframework/kafka/listener/KafkaMessageListenerContainer.java

 						long position = this.consumer.position(tp);
-						Long saved = this.savedPositions.get(tp);
+						OffsetAndMetadata comitted = this.lastCommits.get(tp);


this.lastCommits only contains offsets for TopicPartitions for which records were obtained during the previous poll() call. this.lastCommits is cleared between polls.
This means:

polls returning no records will always trigger a commit for the complete assignment.

polls returning records for some partitions only will trigger commits for the other partitions even when the LSO did not change since the last commit.

Should the fix try to avoid unnecessary commits or is it good enough as is?

@bgK I guess there is a situation where a producer might keep rolling back transactions and the lag will increase until a successful transaction appears in the log.

I must admit that I am not comfortable doing a commit for each empty poll but if the above is true, then there is probably no choice.

I really wish the kafka folks would fix the underlying problem of reporting this bogus lag.

I must admit that I am not comfortable doing a commit for each empty poll but if the above is true, then there is probably no choice.

Me neither. What I was hinting at with this comment is whether Spring-Kafka should remember the last committed offsets across polls to avoid redundant commits.

Right, but as I described, I believe there are cases where the lag can still increase unless we do.

I believe the proper fix is for Kafka to not report a lag if it's a bogus lag due to uncommitted, or rolled-back, records.

Right, but as I described, I believe there are cases where the lag can still increase unless we do.

I agree Spring-Kafka should commit the offsets when the consumer position changed after empty polls, to account for rollbacked transactions. What's unclear to me is what committing the same offsets multiple times when the position does not change after empty polls would accomplish. Do you have any insight?

I believe the proper fix is for Kafka to not report a lag if it's a bogus lag due to uncommitted, or rolled-back, records.

As far as I know, at this point, Kafka does not provide any other mechanism to report the consumer lag accurately when using transactional producers. Shouldn't Spring-Kafka make sure the situations where Kafka reports the lag properly work when using the Spring abstractions?

Shouldn't Spring-Kafka make sure the situations where Kafka reports the lag properly

My point is that the so-called "lag" is not really a lag, and shouldn't be reported as such.

alograg · 2023-06-12T08:17:35Z

Hi @garyrussell
I will participate in the correction.
I understand clearly that the "lag" does not exist but Kafka sends it as such, because it is a bugus lag.
And based on your experience it is better that be fixed in Kafka.
Look for such a report in KAfka JIRA and i only find the report KAFKA-10683 - Consumer.position() Ignores Transaction Marker with read_uncommitted.
Do you have the report number for this behavior?
I plan to correct this problem from both sides.
I will start generating the tests that show this error/behavior.

garyrussell · 2023-06-12T15:05:07Z

Yes; that is the issue that I raised.

alograg · 2023-07-17T15:33:26Z

Dear @garyrussell

I hope finds you well. I wanted to provide you with an update regarding the known bug we discussed in the Kafka developers' mailing list. After interacting with the community, waiting for additional input, and reviewing the code, it appears that this is a known bug without a feasible solution due to the underlying handling logic.

This issue is similar to other processes, such as Log Compaction. Besaids, the community is currently exploring a proposal to refactor the KafkaConsumer.

I was wondering if you have any preferred channel where we can further discuss this matter. I would greatly appreciate your insights and expertise in finding ways to improve the code and document workarounds for this issue.

Please let me know your thoughts and if there is a suitable platform or forum where we can continue this discussion. I look forward to collaborating with you to overcome this challenge.

Thank you for your attention to this matter.

Best regards

garyrussell · 2023-07-17T15:47:49Z

@alograg I am not sure I can add any value to such a discussion, but feel free to use the Discussions tab above.

alograg · 2023-07-18T09:19:06Z

Dicsution

artembilan requested changes May 19, 2023

View reviewed changes

bgK reviewed May 26, 2023

View reviewed changes

alograg force-pushed the main branch 2 times, most recently from efa1472 to 2b4b570 Compare June 12, 2023 08:20

artembilan merged commit 2b4b570 into spring-projects:main Jun 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for issue #2590 #2685

Fix for issue #2590 #2685

thperchi commented May 17, 2023

pivotal-cla commented May 17, 2023

garyrussell commented May 17, 2023

pivotal-cla commented May 19, 2023

thperchi commented May 19, 2023

artembilan left a comment

garyrussell commented May 22, 2023

thperchi commented May 23, 2023

bgK May 26, 2023

garyrussell Jun 1, 2023

bgK Jun 1, 2023

garyrussell Jun 1, 2023

bgK Jun 6, 2023 •

edited

Loading

garyrussell Jun 7, 2023

alograg commented Jun 12, 2023

garyrussell commented Jun 12, 2023

alograg commented Jul 17, 2023

garyrussell commented Jul 17, 2023

alograg commented Jul 18, 2023

Fix for issue #2590 #2685

Fix for issue #2590 #2685

Conversation

thperchi commented May 17, 2023

pivotal-cla commented May 17, 2023

garyrussell commented May 17, 2023

pivotal-cla commented May 19, 2023

thperchi commented May 19, 2023

artembilan left a comment

Choose a reason for hiding this comment

garyrussell commented May 22, 2023

thperchi commented May 23, 2023

bgK May 26, 2023

Choose a reason for hiding this comment

garyrussell Jun 1, 2023

Choose a reason for hiding this comment

bgK Jun 1, 2023

Choose a reason for hiding this comment

garyrussell Jun 1, 2023

Choose a reason for hiding this comment

bgK Jun 6, 2023 • edited Loading

Choose a reason for hiding this comment

garyrussell Jun 7, 2023

Choose a reason for hiding this comment

alograg commented Jun 12, 2023

garyrussell commented Jun 12, 2023

alograg commented Jul 17, 2023

garyrussell commented Jul 17, 2023

alograg commented Jul 18, 2023

bgK Jun 6, 2023 •

edited

Loading