Skip to content

Conversation

leochans
Copy link

when use batch type listener, simply declare a bean of RecordMessageConverter will not effect, should warp the RecordMessageConverter to a BatchMessagingMessageConverter when use batch type listener

when use batch type listener, simple declare a bean of type
RecordMessageConverter will not effect, should warp the
RecordMessageConverter to a BatchMessagingMessageConverter
when use batch type listener
@pivotal-issuemaster
Copy link

@leochans Please sign the Contributor License Agreement!

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

See the FAQ for frequently asked questions.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 15, 2019
@pivotal-issuemaster
Copy link

@leochans Thank you for signing the Contributor License Agreement!

@snicoll
Copy link
Member

snicoll commented Jan 15, 2019

paging @garyrussell

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Jan 15, 2019
@snicoll snicoll changed the title kafka BatchMessagingMessageConverter auto-configuration Auto-configure BatchMessagingMessageConverter with Listener.Type.Batch Jan 15, 2019
@garyrussell
Copy link
Contributor

@snicoll Functionally LGTM but there are code formatting violations.

It also needs a couple of tests - one with, and one without a configured RecordMessageConverter @Bean - it can be null, in which case the listener receives a List<Foo> where Foo is the unconverted ConsumerRecord.value() (List<String> with default Boot configuration); with a converter it gets List<Bar> where the Bars are converted from ConsumerRecord.value().

That said, I think a better solution would be to auto-configure a BatchMessageConverter (alongside the RecordMessageConverter) for use with batch listeners.

That would give the user more control over the converter, rather than hard-wiring a BatchMessagingMessageConverter implementation that wraps the configured RecordMessageConverter.

@leochans
Copy link
Author

@garyrussell I had think about RecordMessageConverter be null thing, when it is null, BatchMessageConverter will not do the convert, see BatchMessagingMessageConverter:163

it's already init a BatchMessageConverter in BatchMessagingMessageListenerAdapter use a new BatchMessagingMessageConverter(), in the BatchMessagingMessageConverter() called this(null)

@garyrussell
Copy link
Contributor

I understand that (I wrote the code); it can be null without problems, but test cases are needed to ensure the configuration works as expected.

However, I think my suggestion to auto-configure a BatchMessageConverter is a better solution (but it could fall back to your code if none is provided, but a RecordConverter is).

@snicoll
Copy link
Member

snicoll commented Feb 13, 2019

Thank you for the PR @leochans but I've decided to take a different approach based on the feedback from @garyrussell, see #15942 for more details.

@snicoll snicoll closed this Feb 13, 2019
@snicoll snicoll added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Feb 13, 2019
@snicoll snicoll removed their assignment Feb 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: declined A suggestion or change that we don't feel we should currently apply

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants