-
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
Fix loading dashboards in ES 5.4 + tests connection timeout increase #4525
Conversation
I started with investigating a flaky test and increasing the connection timeout, but went down the rabbit hole a bit: * The dashboard loader didn't work against 5.4, i think since elastic#4471. This fixes it by checking the version and only setting single_type for > 6.0. * GetTestingElasticsearch now asserts on errors and increases the connection timeout * Several tests were calling Connect twice
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.
Nice job at bug hunting.
libbeat/outputs/elasticsearch/api.go
Outdated
@@ -52,7 +53,7 @@ func (r QueryResult) String() string { | |||
|
|||
func withQueryResult(status int, resp []byte, err error) (int, *QueryResult, error) { | |||
if err != nil { | |||
return status, nil, err | |||
return status, nil, errors.Errorf("%v. Response: %s", err, resp) |
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.
This looks like a good candidate for errors.Wrapf
as opposed to Errorf
.
var address = "http://" + GetEsHost() + ":" + GetEsPort() | ||
username := os.Getenv("ES_USER") | ||
pass := os.Getenv("ES_PASS") | ||
client := newTestClientAuth(address, username, pass) | ||
|
||
// Load version number | ||
client.Connect(3 * time.Second) | ||
err := client.Connect(60 * time.Second) | ||
assert.NoError(t, err) |
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.
It would better to use if err != nil { t.Fatal(e) }
so that the test doesn't run. assert.NoError
is like t.Error
in that the test keeps executing.
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
jenkins, test it |
I started with investigating a flaky test and increasing the
connection timeout, but went down the rabbit hole a bit:
fixes it by checking the version and only setting single_type for > 6.0.
timeout