Skip to content
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

UDP inputs now uses human friendly size to define MaxMessageSize #6886

Merged
merged 2 commits into from Apr 19, 2018

Conversation

Projects
None yet
4 participants
@ph
Copy link
Member

commented Apr 17, 2018

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.

@ph ph added the Filebeat label Apr 17, 2018


import "github.com/dustin/go-humanize"

type Size int64

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Apr 17, 2018

exported type Size should have comment or be unexported

@ph ph force-pushed the ph:fix/humanize-udp branch from cf883ea to 658c7e4 Apr 17, 2018

@@ -3,6 +3,8 @@ package udp
import (
"time"

humanize "github.com/dustin/go-humanize"

This comment has been minimized.

Copy link
@kvch

kvch Apr 17, 2018

Contributor

I might be wrong, but AFAIK the prefix "go-" is removed automatically. So you don't need to alias this import.

This comment has been minimized.

Copy link
@ph

ph Apr 17, 2018

Author Member

I really need to fix my editor to stop adding theses.

This comment has been minimized.

Copy link
@ph

ph Apr 17, 2018

Author Member

@kvch Your vim doesn't do that?

@kvch

kvch approved these changes Apr 17, 2018

@ph ph force-pushed the ph:fix/humanize-udp branch from 658c7e4 to 3942125 Apr 17, 2018

@ph

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2018

@kvch updated

@ph

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2018

jenkins test this please

@ph ph added in progress review and removed in progress review labels Apr 18, 2018

@ph

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2018

@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.

@@ -18,7 +19,7 @@ type client struct {
done chan struct{}
metadata Metadata
splitFunc bufio.SplitFunc
maxReadMessage size
maxReadMessage common.Size

This comment has been minimized.

Copy link
@ruflin

ruflin Apr 18, 2018

Collaborator

Could we rename the variable here to be in line with the config naming? This will make it easier to read the code base.

This comment has been minimized.

Copy link
@ph

ph Apr 18, 2018

Author Member

Will rename it to maxMessageSize

type Size int64

// Unpack convert a size defined from a human readable format into bytes.
func (s *Size) Unpack(value string) error {

This comment has been minimized.

Copy link
@ruflin

ruflin Apr 18, 2018

Collaborator

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.

This comment has been minimized.

Copy link
@ph

ph Apr 18, 2018

Author Member

I am OK to add a deprecation warning.

This comment has been minimized.

Copy link
@ph

ph Apr 18, 2018

Author Member

@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? ...

@ph ph force-pushed the ph:fix/humanize-udp branch 2 times, most recently from 790363b to 77d5271 Apr 18, 2018

@ph ph added review and removed in progress labels Apr 18, 2018

@ph

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2018

@kvch @ruflin renamed updated and added the warning. 👍

@ruflin
Copy link
Collaborator

left a 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.

@@ -12,7 +14,7 @@ var defaultConfig = config{
Type: "udp",
},
Config: udp.Config{
MaxMessageSize: 10240,
MaxMessageSize: 1 * humanize.KiByte,

This comment has been minimized.

Copy link
@ruflin

ruflin Apr 19, 2018

Collaborator

Sorry for the late comment, but I just realised we are changing the default here from 10KB to 1KB?

@ph

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2018

@ph

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2018

@ruflin all update good catch, waiting for green.

@@ -29,13 +29,13 @@ The `udp` input supports the following configuration options plus the
[id="{beatname_lc}-input-{type}-max-message-size"]
==== `max_message_size`

The maximum size of the message received over UDP. The default is `10240`.
The maximum size of the message received over UDP. The default is `1KiB`.

This comment has been minimized.

Copy link
@ruflin

ruflin Apr 19, 2018

Collaborator

@ph I think you forgot the docs. Also see above.

ph added some commits Apr 17, 2018

UDP inputs now uses human friendly size to define MaxMessageSize
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.

@ph ph force-pushed the ph:fix/humanize-udp branch from 28805e1 to cbdc861 Apr 19, 2018

@ph

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2018

@ruflin I've fixed the docs issue and manually rebased to have a cleaner history.

@ruflin

ruflin approved these changes Apr 19, 2018

Copy link
Collaborator

left a comment

WFG

@ruflin ruflin merged commit 227dc95 into elastic:master Apr 19, 2018

4 of 6 checks passed

beats-ci Build started sha1 is merged.
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
CLA Commit author has signed the CLA
Details
Hound No violations found. Woof!
codecov/patch 87.5% of diff hit (target 62.93%)
Details
codecov/project Absolute coverage decreased by -3.62% but relative coverage increased by +24.56% compared to 8e29a8b
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.