-
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
Add option to adjust _source options + improvements #4317
Conversation
eb48b26
to
a9fc7d1
Compare
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.
LGTM
libbeat/template/config.go
Outdated
Overwrite bool `config:"overwrite"` | ||
OutputToFile string `config:"output_to_file"` | ||
Settings map[string]interface{} `config:"settings"` | ||
SourceSettings map[string]interface{} `config:"_source"` |
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.
At first I was also sceptical about using _source but I think for ES users it must be quite intuitive to use it like this.
libbeat/template/template.go
Outdated
index string | ||
beatVersion Version | ||
esVersion Version | ||
settings common.MapStr |
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.
Seeing both names I was thinking if it could be settings._source
and settings.x
but I would not know how to call x
:-(
Is there are stuff we potentially going to add here?
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.
I can imagine there could be more yes (e.g. _all), although I don't plan more at the moment. We could maybe use index
for x
, so you'd have: settings.index.number_of_shards
and settings._source.enabled: false
. Trying to figure out if there can be any settings that do not fall under index
...
I've updated the code to have
and
With this change, we are can only use the second version. I think that's good anyway, as we don't want two ways of accomplishing the same thing. |
libbeat/template/config.go
Outdated
Settings TemplateSettings `config:"settings"` | ||
} | ||
|
||
type TemplateSettings struct { |
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.
[golint] reported by reviewdog 🐶
exported type TemplateSettings should have comment or be unexported
libbeat/template/config.go
Outdated
Settings TemplateSettings `config:"settings"` | ||
} | ||
|
||
type TemplateSettings struct { |
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.
[golint] reported by reviewdog 🐶
type name will be used as template.TemplateSettings by other packages, and that stutters; consider calling this Settings
This adds/fixes: * Ability to disable _source, or set other _source related options * Moves template index settings under `settings.index` * Fixes the overwrite logic (was using the wrong template name on the check) * Fixes error handling * Integration tests for overwritting Part of elastic#3654 and elastic#4112.
289450b
to
3072a79
Compare
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.
I like the new structure with the index prefix of the settings. Left a minor docs comment.
@@ -35,3 +35,17 @@ setup.template.settings: | |||
index.number_of_shards: 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.
Should this be updated to put index
on the line above?
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.
I kind of like it like it is now because it matches the Elasticsearch docs where they usually talk about index.*
as the name of the options: https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping.html
This PR adds/fixes:
The source can be disabled like this:
And _source can be also partially enabled via "includes" and "excludes", for example:
Adding the discuss label for naming. I hesitated between using
_source
orsource_settings
for these options, but I like the look of_source.enabled: false
.Part of #3654 and #4112.
Remaining TODOs: