-
Notifications
You must be signed in to change notification settings - Fork 636
GH-1032: Consumer Batching Phase 2 @RabbitListener #1037
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
converted = new GenericMessage<>(messages); | ||
} | ||
else { | ||
List<Message<?>> messagingMessages = messages.stream() |
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.
May we avoid Java Streams on critical Framework paths?
factory.setBatchListener(true); | ||
factory.setContainerConfigurer(container -> { | ||
container.setConsumerBatchEnabled(true); | ||
container.setDeBatchingEnabled(true); |
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.
Why these two options can't be populated directly by the SimpleRabbitListenerContainerFactory
when that etBatchListener(true)
is used?
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.
Because the original purpose of batchListener
was for producer batches.
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.
M-m-m. Can't we cover those "producer batches" with a condition based on the BatchingStrategy
provided?
src/reference/asciidoc/amqp.adoc
Outdated
In addition, a new property `consumerBatchEnabled` has been added to the `SimpleMessageListenerContainer`. | ||
When this is true, the container will create a batch of messages, up to `batchSize`; a partial batch is delivered if `receiveTimeout` elapses with no new messages arriving. | ||
If a producer-created batch is received, it is debatched and added to the consumer-side batch; therefore the actual number of messages delivered may exceed `batchSize`, which represents the number of messages received from the broker. | ||
`deBatchingEnabled` must be true when `consumerBatchEnabled` is true. |
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.
Why can't this happen automaticaly?
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.
Because batchListener
on the factory serves 2 purposes.
- configures the
BatchMessagingMessageListenerAdapter
- sets deBatchingEnabled to false (for producer batches).
I was not comfortable with this either; I think I need to add consumerBatchEnabled
as a first class property on the factory.
Will push another commit.
Resolves spring-projects#1032 Add basic consumer batching support to the `@RabbitListener` infrastructure.
* Add a test to ensure producer debatching works for DMLC too
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.
I think we are very close.
|
||
/** | ||
* @param batchSize the transaction size. | ||
* @see SimpleMessageListenerContainer#setBatchSize |
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.
Should we say @since 2.2
?
Resolves #1032
Add basic consumer batching support to the
@RabbitListener
infrastructure.Still needs more tests But I think this can be merged after review.