-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 Elasticsearch version flag #1753
Add Elasticsearch version flag #1753
Conversation
Signed-off-by: Pavol Loffay <[email protected]>
Signed-off-by: Pavol Loffay <[email protected]>
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 - just a couple of minor things.
pkg/es/mocks/Client.go
Outdated
@@ -1,24 +1,11 @@ | |||
// Code generated by mockery v1.0.0. DO NOT EDIT. | |||
|
|||
// Copyright (c) 2019 The Jaeger Authors. |
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.
Was this removed by the autogeneration?
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.
yes, the fmt goal adds it back
plugin/storage/es/options.go
Outdated
@@ -96,6 +97,7 @@ func NewOptions(primaryNamespace string, otherNamespaces ...string) *Options { | |||
TagDotReplacement: "@", | |||
Enabled: true, | |||
CreateIndexTemplates: true, | |||
Version: 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.
nit: format
plugin/storage/es/options.go
Outdated
flagSet.Uint( | ||
nsConfig.namespace+suffixVersion, | ||
0, | ||
"The major Elasticsearch version. The not specified value enables auto-detection from Elasticsearch API.") |
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.
"The not specified value ..." -> "If not specified, the value will be auto-detected from Elasticsearch"?
@@ -40,16 +36,6 @@ type esTokenPropagationTestHandler struct { | |||
} | |||
|
|||
func (h *esTokenPropagationTestHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { | |||
// Return the elasticsearch version |
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.
Was this removed because it was a temporary workaround when the version wasn't available?
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.
yes
Signed-off-by: Pavol Loffay <[email protected]>
The version parameter was added to rollover in this PR #1769 |
Follow up on #1690 (comment)
The elasticsearch version is by default automatically derived from ES API. I have added a flag which allows to specify the version. This can be used if for some reason the ping ES API is not available or it allows to use ES7 template with ES 6.8.x for easier migration to ES 7 (just wait for old indices to TLLout).