-
Notifications
You must be signed in to change notification settings - Fork 289
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
Elasticsearch v8 Support #1215
Elasticsearch v8 Support #1215
Conversation
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.
Works well for both v7 and v8 👍 Please see one small comment before merge. Thanks!
pkg/sink/elasticsearch.go
Outdated
if err != nil && err.(*elastic.Error).Details.Type != "resource_already_exists_exception" { | ||
return fmt.Errorf("while creating index: %w", 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.
if err != nil && err.(*elastic.Error).Details.Type != "resource_already_exists_exception" { | |
return fmt.Errorf("while creating index: %w", err) | |
} | |
if err != nil && elastic.ErrorReason(err) != "resource_already_exists_exception" { | |
return fmt.Errorf("while creating index: %w", err) | |
} |
BTW please move this reason to consts 👍
pkg/sink/elasticsearch.go
Outdated
return fmt.Errorf("while creating index: %w", err) | ||
} | ||
} | ||
|
||
// Send event to els | ||
_, err = e.client.Index().Index(indexName).Type(indexCfg.Type).BodyJson(event).Do(ctx) | ||
indexService := e.client.Index().Index(indexName) | ||
if strings.HasPrefix(e.clusterVersion, "7.") { |
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.
BTW, shouldn't we also check for versions below? E.g. 6? WDYT?
Maybe we can split this string by .
, parse the first item as int and see whether is 7 or below?
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 a good question :) Let me add check for that
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.
Added
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.
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.
Thank you @huseyinbabal! Looks great 🚀
Description
Changes proposed in this pull request:
Testing
v8 Test
v7 Test
In both tests, you shouldn't see 400 errors.
Related issue(s)
#887