UDP inputs now uses human friendly size to define MaxMessageSize#6886
UDP inputs now uses human friendly size to define MaxMessageSize#6886ruflin merged 2 commits intoelastic:masterfrom ph:fix/humanize-udp
Conversation
libbeat/common/size.go
Outdated
There was a problem hiding this comment.
exported type Size should have comment or be unexported
filebeat/input/udp/config.go
Outdated
There was a problem hiding this comment.
I might be wrong, but AFAIK the prefix "go-" is removed automatically. So you don't need to alias this import.
There was a problem hiding this comment.
I really need to fix my editor to stop adding theses.
|
@kvch updated |
|
jenkins test this please |
|
@kvch don't merge it yet, I have second thoughts about the name, I think It will go with byteSize instead of size it will a bit more explicit. |
filebeat/inputsource/tcp/client.go
Outdated
There was a problem hiding this comment.
Could we rename the variable here to be in line with the config naming? This will make it easier to read the code base.
There was a problem hiding this comment.
Will rename it to maxMessageSize
libbeat/common/size.go
Outdated
There was a problem hiding this comment.
I like this as a temporarely fix to make migration easier. But I would remove it again in 7.0 to only have 1 way to configure the values.
This would mean to when someone uses it without a unit, we should log a deprecation warning.
There was a problem hiding this comment.
I am OK to add a deprecation warning.
There was a problem hiding this comment.
@ruflin When I tried to use cfgwarn in that file, I hit aimport cycle not allowed, to fix that I have either to move theses new common type out of common OR maybe cfgwarn out of common and have a backward compatible alias? ...
ruflin
left a comment
There was a problem hiding this comment.
I would keep the current default of 10KB as a mutline event can reach that limit pretty easily I'm thinking. If we change it, we should add it to the changelog.
Code LGTM.
filebeat/input/udp/config.go
Outdated
There was a problem hiding this comment.
Sorry for the late comment, but I just realised we are changing the default here from 10KB to 1KB?
|
facepalm
On Thu, Apr 19, 2018 at 5:33 AM Nicolas Ruflin ***@***.***> wrote:
***@***.**** commented on this pull request.
I would keep the current default of 10KB as a mutline event can reach that
limit pretty easily I'm thinking. If we change it, we should add it to the
changelog.
Code LGTM.
------------------------------
In filebeat/input/udp/config.go
<#6886 (comment)>:
> @@ -12,7 +14,7 @@ var defaultConfig = config{
Type: "udp",
},
Config: udp.Config{
- MaxMessageSize: 10240,
+ MaxMessageSize: 1 * humanize.KiByte,
Sorry for the late comment, but I just realised we are changing the
default here from 10KB to 1KB?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#6886 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAACgEmxtYIoqVDiS9fEqpn-7yK5dIFOks5tqFnOgaJpZM4TYneA>
.
--
ph
|
|
@ruflin all update good catch, waiting for green. |
There was a problem hiding this comment.
@ph I think you forgot the docs. Also see above.
Uses go-humanize to parse values for `MaxMessageSize` and fallback to raw bytes if no suffix is found. Also create a new type for future configuration named `cfgtype.ByteSize` that will take care of correctly unpacking values.
|
@ruflin I've fixed the docs issue and manually rebased to have a cleaner history. |
Uses go-humanize to parse values for
MaxMessageSizeand fallback toraw bytes if no suffix is found. Also create a new type for future
configuration named
cfgtype.ByteSizethat will take care of correctlyunpacking values.