Hey!

I think@tyx has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

Hello @symfony/mergers

The initial issue got 8 👍🏼 and, this PR got 5 👍🏼 and@jderusse liked this feature.

So without feedback, I'm planning to merge this PR by the end of the month, before the feature freeze.
If you are against it, it's time to raise your voice :)

This is indeed a feature I would like to see in 5.4. 👍

Btw,@lyrixx any chance you could also take a look at my#42335 before the feature freeze is upon us? 😅

I have rebased the PR (to fix conflict on CHANGELOG.md file)

Instead, what about adding a middleware that does the buffering, and communicates with the worker via a stamp as done here? Buffering would be enabled/disabled by either setting a limit on the number of messages per batch, or by injecting a buffering decision strategy if we want something super flexible.

Hmm, indeed it might be possible. But ATM, I fail to see how the middleware could actually call the "code that process the buffer". Should we introduce a new method (via an interface) on the handler (Like it's done on monolog handler)?

So basically, you want me to move the theConfigurableAutoAckInterface::isAutoAckDisabled() to a middleware instead of the handler itself ? (I'm asking to be sure I fully understand)

BTW, how are we sure that messages are flushed eventually, when we don't know when messages can arrive? Should we inject a delayed message that would trigger the flush if nothing happened before it?

As you can see in the PR body, user need to register also a listener but a timer to flush after a configured time. But this could leave in the middleware

About naming, "DelayedAckStamp" made me think: "delayed? when?". Maybe this should be named "DisableAutoAckStamp"?

I agree 👍🏼

But I see a drawback with your idea: a new middleware implies a new bus because one doesn't want to buffer all message:https://symfony.com/doc/current/messenger.html#middleware

So publishing a message will be more complexe because one will have to choose the right buses when dispatching the message. IMHO, it's error prone.

So you want to move the "handlers [is] aware of the transport layer" to the publisher is aware of the transport layer. Is it really better?

how the middleware could actually call the "code that process the buffer".

can't the middleware call the handler many times in a loop, once per buffered message?
the middleware would do that when the Nth message arrives (aka when the buffer is full).

So basically, you want me to move the the ConfigurableAutoAckInterface::isAutoAckDisabled() to a middleware instead of the handler itself ?

I think we won't need any new interface if we do this. The stamp might be enough.

a new middleware implies a new bus because one doesn't want to buffer all message

we could have a config option that gives the size of the buffer, and if the size is > 1, then we add this middleware? (or we remove it if it's <= 1? or we make it a no-op if we can't skip registering it).

can't the middleware call the handler many times in a loop, once per buffered message?
the middleware would do that when the Nth message arrives (aka when the buffer is full).

nope, because you don't know how it's the "last message". You need all messages at once.

I think we won't need any new interface if we do this. The stamp might be enough.

You don't have access to the stamp in the handler. Adding it is what you don't want.

we could have a config option that gives the size of the buffer, and if the size is > 1, then we add this middleware? (or we remove it if it's <= 1? or we make it a no-op if we can't skip registering it).

I don't understand. Middlerware a setup for a busses. not for a transport or handler. See the doc to better understand.

Here is another proposal on the topic:#43354

Closing in favor of#43354, thanks for pushing this@lyrixx!