-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat: Replace LinkedList with ArrayList #3766
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
Conversation
As per spring-projects#3764 Signed-off-by: Janek Lasocki-Biczysko <[email protected]>
9cca98b
to
665a345
Compare
...ng-kafka/src/main/java/org/springframework/kafka/listener/KafkaMessageListenerContainer.java
Outdated
Show resolved
Hide resolved
...ng-kafka/src/main/java/org/springframework/kafka/listener/KafkaMessageListenerContainer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We got a failed test with your change:
SubBatchPerPartitionTests > withFilter() FAILED
org.opentest4j.AssertionFailedError:
Expecting value to be true but was false
at app//org.springframework.kafka.listener.SubBatchPerPartitionTests.withFilter(SubBatchPerPartitionTests.java:121)
And you indeed mentioned that in the issue.
WDYT?
Apologies -- I raised the PR too quickly. I assumed using |
spring-kafka/src/main/java/org/springframework/kafka/listener/adapter/RecordFilterStrategy.java
Outdated
Show resolved
Hide resolved
...ng-kafka/src/main/java/org/springframework/kafka/listener/KafkaMessageListenerContainer.java
Outdated
Show resolved
Hide resolved
90da67f
to
c64a5f8
Compare
Signed-off-by: Janek Lasocki-Biczysko <[email protected]>
Signed-off-by: Janek Lasocki-Biczysko <[email protected]>
c64a5f8
to
2e3e2a9
Compare
...ng-kafka/src/main/java/org/springframework/kafka/listener/KafkaMessageListenerContainer.java
Outdated
Show resolved
Hide resolved
spring-kafka/src/main/java/org/springframework/kafka/listener/adapter/RecordFilterStrategy.java
Outdated
Show resolved
Hide resolved
Turns out my comment here #3764 (comment) was totally wrong. I misunderstood the implementation of |
588125d
to
3154948
Compare
3154948
to
14e385e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Will be merged (and back-ported) when all tests pass.
Thank you for your contribution!
Wonderful, thank you for helping me getting over the line. When should I expect a new 3.3.x release? |
See our Release Calendar for March: https://spring.io/projects#release-calendar |
Initialise the "batch of records" list container as an ArrayList rather than LinkedList.
Using a LinkedList has an impact on performance as outlined here #3764
(fixes #3764)