moquette-io / moquette

Java MQTT lightweight broker
http://moquette-io.github.io/moquette/
Apache License 2.0
2.27k stars 814 forks source link

immediate_buffer_flush should be in configuration file, defaulting to true. #718

Closed jflamy closed 1 year ago

jflamy commented 1 year ago

Expected behavior

Correct performance behavior, similar to Mosquitto or Aedes (less than 20ms for message to propagate from emitter to subscriber on localhost). The parameter immediate_buffer_flush is not listed in the default conf file, and not mentioned in the documentation. The default value does not seem appropriate for normal use cases.

Actual behavior

Requests are buffered, resulting in delays of 1 second.

Steps to reproduce

Run with default configuration.

Minimal yet complete reproducer code (or URL to code) or complete log file

Moquette MQTT version

0.16, 0.17-SNAPSHOT

JVM version (e.g. java -version)

n/a

OS version (e.g. uname -a)

n/a

dentex commented 1 year ago

Nice catch! adding props.put(BrokerConstants.IMMEDIATE_BUFFER_FLUSH_PROPERTY_NAME, "true"); I saved ~1 sec, as you said.

andsel commented 1 year ago

That flag is used to force the flush on every channel write operation.

https://github.com/moquette-io/moquette/blob/3c53bd848fe2868823a8facbfdbb01b5efc2d909/broker/src/main/java/io/moquette/broker/MQTTConnection.java#L534-L538

This means that avoid to batch writes and let the underlying network library to better usage the resources. This implies that for every write there is also a flush syscall, which on heavy data situations could potentially impact the performance in term of event per second.

With flush on every write the latency reduces obviously, so it's matter of the use case, if set or not such flag.

jflamy commented 1 year ago

Perhaps the buffering delay should be configurable.

andsel commented 1 year ago

Actually there is an autoflusher filter on the Netty pipeline that after a period (1 sec) of write idle command a flush.

https://github.com/moquette-io/moquette/blob/3c53bd848fe2868823a8facbfdbb01b5efc2d909/broker/src/main/java/io/moquette/broker/NewNettyAcceptor.java#L272

We could just make it configurable, but which is the expected value ranges? Max 5 seconds? Do 0 means "no flush" ? WDYT?

jflamy commented 1 year ago

That would work. And making it more obvious. When switching from Mosqutto or Aedes -- neither does buffering -- this Moquette behaviour looks like a bug. I would actually switch the behavior around. I don't think many people do high volume. They could pick a buffering value that makes sense for their special case. In other words, make it configurable, and make 0 the default :-)

andsel commented 1 year ago

Oks, thanks' for the suggestion I'll try to keep in the radar for 0.17. Just one question, when you say:

make it configurable, and make 0 the default

What means 0? "immediate flush" or "no flush at all, just flush when the write buffer are full" ?

jflamy commented 1 year ago

Same behaviour as Mosquitto or Aedes. Delay before flushing 0 = no buffering, immediate mode. I guess flush when buffer full could be -1.

hylkevds commented 1 year ago

Having 0=immediate flush and -1=when buffer full makes sense. It would be good to be able to configure this in milliseconds. 1 Second is a long time in some use-cases, but buffering for 100ms may be beneficial.

Just noticed that #738 does just that :)