-
Notifications
You must be signed in to change notification settings - Fork 818
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
Switched boolean flag to immediate flushing to integer #738
Switched boolean flag to immediate flushing to integer #738
Conversation
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.
Looks good.
You've not updated the autoflush handler in NewNettyAcceptor yet, right?
@hylkevds have to do, I'm going with incremental commits, to make it easy reviewable. Still work in progress, once ready I'll ask you as reviewer. |
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.
Looks good!
if (props.boolProp(BrokerConstants.IMMEDIATE_BUFFER_FLUSH_PROPERTY_NAME, true)) { | ||
bufferFlushMillis = BrokerConstants.IMMEDIATE_BUFFER_FLUSH; | ||
} else { | ||
bufferFlushMillis = BrokerConstants.NO_BUFFER_FLUSH; |
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.
Maybe it's better to default to the 1s value that used to be used when immediate_buffer_flush
was false
?
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.
Good point, done 👍
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.
If you default to 1000ms people coming from Mosquitto will keep reporting bad performance
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.
The old immediate_buffer_flush
default value is now switched from false
to true
.
So by default it now flushes immediatly, then if the user customize the immediate_buffer_flush
to true, the flush interval is 1000ms
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.
You mean false I presume.immediate False = 1000; immediate true = 0. Explicit numerical value ignored if immediate is true.
…mmediate_buffer_flush'
9710040
to
09ce6e2
Compare
Release notes
Introduces the ability to specify the flush interval for IO write operations, changing the default, to an immediate flush.
What does it do?
immediate_buffer_flush
from boolean to a number (expressing milliseconds)buffer_flush_millis
settings to initialize the auto flusher.Fixes #718