-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Config cleanup #1903
Config cleanup #1903
Conversation
ruflin
commented
Jun 24, 2016
•
edited
Loading
edited
- Code cleanup
- Add humanize for buffersize
112383d
to
c07e922
Compare
@@ -22,7 +22,9 @@ type Filebeat struct { | |||
|
|||
// New creates a new Filebeat pointer instance. | |||
func New() *Filebeat { | |||
return &Filebeat{} | |||
return &Filebeat{ | |||
config: &cfg.DefaultConfig, |
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.
don't take the default config as pointer. DefaultConfig normally serves as template to be copied and must not contain any pointers!
Rather allocate new config initialized with DefaultConfig like:
config := cfg.DefaultConfig
return &Filebeat{ config: &config }
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.
Fixed
@urso New version pushed. |
PublishAsync bool `config:"publish_async"` | ||
IdleTimeout time.Duration `config:"idle_timeout"` | ||
IdleTimeout time.Duration `config:"idle_timeout" validate:"nonzero,min=0"` |
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.
min=1
?
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.
what if I want to disable idle_time
, e.g. via -1 or 0?
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.
Not sure if that is something that we should encourage. That above allows to set it for example to 0.001s which is almost like disabling it.
One thing I just realised related to duration and ucfg: it should be min=1s
and not min=1
as if the unit is not know, it could be "anything".
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.
that's right. if not setting any unit, the default is 's'. But normally I set units.
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.
but in past `idle_timeout' was disablable, no?
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.
@urso didn't know it allows to set units there. Nice :-) Will add units.
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.
default before (and now) is 5s
* Code cleanup * Add humanize for buffersize