Fix ackDiscarded. #3212
Fix ackDiscarded. #3212
Conversation
| @@ -340,6 +340,11 @@ public void setAckDiscarded(boolean ackDiscarded) { | |||
| this.ackDiscarded = ackDiscarded; | |||
| } | |||
|
|
|||
| public void useRecordFilterStrategy(RecordFilterStrategy<? super K, ? super V> recordFilterStrategy) { | |||
| setRecordFilterStrategy(recordFilterStrategy); | |||
| setAckDiscarded(true); | |||
There was a problem hiding this comment.
This new method does not make too much sense since we simply can do this setAckDiscarded(true) in the existing setRecordFilterStrategy().
WDYT?
There was a problem hiding this comment.
I agree with you, i made new commit to follow your guideline.
It is good point! 👍
I was just worried that including setAckDiscarded(true) in setFilterStrategy(...) might bring unintended side effects. For example, a developer might not expect that ackDiscarded would change when updating setFilterStrategy(), since it's not specified in the method signature.
|
@chickenchickenlove, Can you add your name as an author to the classes and update any copyright years? |
| @@ -326,6 +326,7 @@ public void setReplyTemplate(KafkaTemplate<?, ?> replyTemplate) { | |||
| @SuppressWarnings("unchecked") | |||
| public void setRecordFilterStrategy(RecordFilterStrategy<? super K, ? super V> recordFilterStrategy) { | |||
| this.recordFilterStrategy = (RecordFilterStrategy<K, V>) recordFilterStrategy; | |||
| setAckDiscarded(true); | |||
There was a problem hiding this comment.
Looking deeper, I don't think this fix is correct.
See more further:
/**
* Create an instance with the supplied strategy and delegate listener.
* @param delegate the delegate.
* @param recordFilterStrategy the filter.
* @param ackDiscarded true to ack (commit offset for) discarded messages when the
* listener is configured for manual acks.
*/
public FilteringMessageListenerAdapter(MessageListener<K, V> delegate,
RecordFilterStrategy<K, V> recordFilterStrategy, boolean ackDiscarded) {
Therefore it is up to end-user to decide if ackDiscarded or not.
So, what the problem we are trying to fix here, please?
There was a problem hiding this comment.
I see 👍
How about fix the java docs of setAckDiscarded 's Set to true if the {@link #setRecordFilterStrategy(RecordFilterStrategy)} is in use. ?
For example, Set to true if the {@link #setRecordFilterStrategy(RecordFilterStrategy)} is in use and ack discarded messages?
There was a problem hiding this comment.
How about:
Set to true if the {@link #setRecordFilterStrategy(RecordFilterStrategy)} should ack discarded messages.
?
There was a problem hiding this comment.
It's more suitable. (sorry i'm not good at english well🥲 ).
may i fix it? or it's okay to close this PR if you think that it is not needed.
There was a problem hiding this comment.
Yes, please.
Just part of this PR is OK.
Thanks
There was a problem hiding this comment.
Thanks a lot 🙇♂️
I updated It!
|
Thanks for you to review this 🙇♂️
No, there is no opened issue! I found it while investigating other issue. |
Background
descriptionsandmethodsrelated toackDiscardedbehave differently during investigating issue.spring-kafka/spring-kafka/src/main/java/org/springframework/kafka/config/AbstractKafkaListenerEndpoint.java
Lines 335 to 341 in 1d0a7d3
setRecordFilterStrategy()in use,ackDiscardedshould betrue. however,ackDiscardedis alwaysfalseeven if someBeanPostProcessorcallsetRecordFilterStrategy()to useRecordFilter.Changes
useRecordFilterStrategy()to bindsetRecordFilterStrategy()andsetAckDiscarded()to follow java docs.