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

Load the ES template on connect. #1381

Merged
merged 1 commit into from Apr 19, 2016

Conversation

Projects
None yet
3 participants
@tsg
Copy link
Collaborator

commented Apr 13, 2016

It used to try loading it only once on init, causing bug #1321.
This change moves the call to loadTemplate at connection time, immediately
after successful connection. This has the effect that if overwrite is true,
the template will be loaded on each new established connection.

@tsg

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 13, 2016

@urso would be good if you could check for side effects of this, or if you can think of a better way to implement it. I had to do two nested closures, so I'd like to simplify it if possible.

Another issue is that currently loadTemplate ignores errors (apart from logging them). We might want to make Connect() fail if the template loading failed, as running Beats without a template is always bad.

@ruflin ruflin added the libbeat label Apr 13, 2016

@@ -61,6 +62,7 @@ func NewClient(
esURL, index string, proxyURL *url.URL, tls *tls.Config,
username, password string,
params map[string]string,
onConnectCallback *func(client *Client),

This comment has been minimized.

Copy link
@urso

urso Apr 14, 2016

Collaborator

we've got so many parameters in NewClient already, we might consider to pass a config struct to clean this up a little. Pretty annoying to adjust tests every single parameter we add (even if not used by tests)

This comment has been minimized.

Copy link
@urso

urso Apr 14, 2016

Collaborator

onConnectCallback does not need to be a pointer to a function. Just use func (client *Client). One can check for == nil and != nil on func objects only.

This comment has been minimized.

Copy link
@tsg

tsg Apr 15, 2016

Author Collaborator

Cool, didn't know that.

connected bool
http *http.Client
connected bool
onConnectCallback func()

This comment has been minimized.

Copy link
@urso

urso Apr 14, 2016

Collaborator

consider func() error for callback.

This comment has been minimized.

Copy link
@tsg

tsg Apr 15, 2016

Author Collaborator

Done.

@@ -82,6 +84,12 @@ func NewClient(
index: index,
params: params,
}

client.Connection.onConnectCallback = func() {

This comment has been minimized.

Copy link
@urso

urso Apr 14, 2016

Collaborator

wrapping func once again is a little annoying. Why not have type connectCallback func(*Client) error and reuse type all over the place with var noneCallback connectCallback = func(*Client) error { return nil}. Set noneCallback on &Client and if parameter != nil assign it.

This comment has been minimized.

Copy link
@tsg

tsg Apr 15, 2016

Author Collaborator

I gave this a try, but to my taste the code was becoming less readable. Part of the reason is that I have two types of callbacks, one with a client parameter, one without. That's because in Connection I don't have a pointer to the Client, so I used another closure to set it.

Perhaps an easier approach would be just have a pointer to the client in the connection.

@@ -123,17 +121,14 @@ func (out *elasticsearchOutput) init(

// loadTemplate checks if the index mapping template should be loaded
// In case template loading is enabled, template is written to index
func loadTemplate(config Template, clients []mode.ProtocolClient) {
func loadTemplate(config Template, client *Client) {

This comment has been minimized.

Copy link
@urso

urso Apr 14, 2016

Collaborator

with multiple elasticsearch hosts being configured, old implementation used to use first configured host only (drawback: what if first configured host is unresponsive). This change ensures 'loadTemplate' is executed per configured host.

There is a big chances of clients racing to configure the template. Is this potentially a problem? If so, consider using a mutex to linearize template loading. Still, every client must have the chance to load(check) the template, no matter the result for other clients to load the template.

This comment has been minimized.

Copy link
@tsg

tsg Apr 15, 2016

Author Collaborator

Yeah, I think it's safest to do this for each host, because inserting even one document before the template is loaded makes the whole effort useless.

I don't think the races can cause problems because we're loading the same template, but adding a mutex shouldn't hurt either.

@urso

This comment has been minimized.

Copy link
Collaborator

commented Apr 14, 2016

I think error handling in loadTemplate is required. What if shield is available, and beats do not have rights to load template?

@tsg tsg force-pushed the tsg:load_template_on_connect branch from 400541c to bcb390c Apr 15, 2016

@tsg

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 18, 2016

Improvements since last review:

  • Error checking and mark the Connect as failed if the template cannot be PUT to Elasticsearch
  • Read the template file on startup and exit with an error if the file cannot be found (also happens when using -configtest).
  • Serialize loadTemplates requests with a Mutex
  • Added an integration test for template loading

@urso, can you have another look?

@urso

This comment has been minimized.

Copy link
Collaborator

commented Apr 18, 2016

LGTM, but travis seems to complain.

@tsg tsg force-pushed the tsg:load_template_on_connect branch from 270ac1d to 9439e4d Apr 18, 2016

@tsg

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 18, 2016

Rebased and added the Changelog item. The test failure in Jenkins doesn't seem related.

@tsg

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 18, 2016

jenkins, retest it

Load the ES template on connect.
It used to try loading it only once on init, causing bug #1321.
This change moves the call to loadTemplate at connection time, immediately
after successful connection. This has the effect that if overwrite is true,
the template will be loaded on each new established connection.

The template is read on init time and sent to Elasticsearch at connect time.
This means that if the template path is wrong, it will be discovered at
startup (including `-configtest`).

In case there is an error loading the template, the Connect call fails.

This commit includes an integration test for the behaviour.

@tsg tsg force-pushed the tsg:load_template_on_connect branch from 9439e4d to bc53cdb Apr 18, 2016

@ruflin ruflin merged commit d601de2 into elastic:master Apr 19, 2016

4 checks passed

CLA Commit author is a member of Elasticsearch
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details

@tsg tsg deleted the tsg:load_template_on_connect branch Aug 25, 2016

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.